Skip to content

Commit

Permalink
fix(editor): don't create a duplicate import for aliased files (#6062)
Browse files Browse the repository at this point in the history
**Problem:**
Currently when trying to import the same component from an import source
that is aliased in the file, we incorrectly recognize it as a different
source and create a duplicate import

**Fix:**
Take the alias mapping into consideration. The heavy lifting was done in
the previous prep PR which was to pass this mapping into the
`mergeImports` function.

**To reproduce/test:**
In our sample store project, locate the `Grid` with the six
`ProductCard`s, and try to move (simply move) one of the `ProductCard`
inside that grid. It used to create a duplicate import.

**Manual Tests:**
I hereby swear that:

- [X] I opened a hydrogen project and it loaded
- [X] I could navigate to various routes in Preview mode

Fixes #5980
  • Loading branch information
liady committed Dec 13, 2024
1 parent 0280fd3 commit 7d4033a
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 2 deletions.
4 changes: 3 additions & 1 deletion editor/src/core/shared/import-shared-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { importAlias, importDetails } from './project-file-types'
import { absolutePathFromRelativePath } from '../../utils/path-utils'
import { stripExtension } from '../../components/custom-code/custom-code-utils'
import type { FilePathMappings } from '../model/project-file-utils'
import { applyFilePathMappingsToFilePath } from '../workers/common/project-file-utils'

export function renameDuplicateImports(
existingImports: Imports,
Expand All @@ -13,7 +14,8 @@ export function renameDuplicateImports(
): ImportsMergeResolution {
function absolutePath(relativePath: string): string {
const rawAbsolutePath = absolutePathFromRelativePath(targetFilePath, false, relativePath)
const absoluteImportSource = stripExtension(rawAbsolutePath)
const afterFilePathMapping = applyFilePathMappingsToFilePath(rawAbsolutePath, filePathMappings)
const absoluteImportSource = stripExtension(afterFilePathMapping)
return absoluteImportSource
}
const existingNames = getAllImportsUniqueNames(existingImports)
Expand Down
19 changes: 19 additions & 0 deletions editor/src/core/workers/common/project-file-utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,25 @@ describe('mergeImports', () => {
})
})

it('does not duplicate same import from aliased sources', () => {
const result = mergeImports(
'/src/code.js',
[[/@h2\/([^/]*)/y, ['/app/components/hydrogen/$1']]],
{ '@h2/ProductCard': importDetails(null, [importAlias('Card')], null) },
{
'/app/components/hydrogen/ProductCard.jsx': importDetails(
null,
[importAlias('Card')],
null,
),
},
)
expect(result.imports).toEqual({
'@h2/ProductCard': importDetails(null, [importAlias('Card')], null),
})
expect(result.duplicateNameMapping).toEqual(new Map())
})

it('handles duplicate imports', () => {
const result = mergeImports(
'/src/code.js',
Expand Down
3 changes: 2 additions & 1 deletion editor/src/core/workers/common/project-file-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ export function mergeImports(

allImportSources.forEach((importSource) => {
const rawAbsolutePath = absolutePathFromRelativePath(fileUri, false, importSource)
const absoluteImportSource = stripExtension(rawAbsolutePath)
const unaliasedPath = applyFilePathMappingsToFilePath(rawAbsolutePath, filePathMappings)
const absoluteImportSource = stripExtension(unaliasedPath)
if (fileUriWithoutExtension === absoluteImportSource) {
// Prevent accidentally importing the current file
return
Expand Down

0 comments on commit 7d4033a

Please sign in to comment.