Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let package export maps take priority over normal resolution #1781

Merged
merged 4 commits into from
Dec 2, 2020

Conversation

drwpow
Copy link
Collaborator

@drwpow drwpow commented Dec 2, 2020

Changes

Consider the following export map:

  "exports": {
    "./assets/backup.svg":"./dist/assets/backup.svg"
  }

If you imported foo/assets/backup.svg, that should resolve to foo/dist/assets/backup.svg as indicated by the export map. However current behavior was that esinstall would assume that foo/assets/backup.svg was the full path and it would fail to locate the asset.

This PR still lets esinstall try and resolve full paths, but now we check the package exports first for a match. If that exists, and we find a match, use that. If not, the behavior continues as usual. Overall this is a minor change in behavior, and shouldn’t disrupt any packages (the only possible breaking change would be if a package had both a valid package export AND a file at that path, but that would be “wrong” anyway and should be fixed by this, as that’s not expected behavior).

Testing

Docs

@vercel
Copy link

vercel bot commented Dec 2, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/rdhl23f04
✅ Preview: https://snowpack-git-drwpow-export-map-priority.pikapkg.vercel.app

"preact"
"preact",
"preact/hooks",
"@clevercloud/components/assets/backup.svg"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds a local test for the new behavior (the others are just snapshot updates)

const linkElement = getByText(/learn react/i);
expect(document.body.contains(linkElement));
});
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn’t need these tests

* one
* two
* three
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test wasn’t working, and now it is?

@@ -542,7 +378,7 @@ function asyncEachOfLimit(generator, limit, iteratee, callback) {
}
var eachOfLimit = (limit) => {
return (obj, iteratee, callback) => {
callback = once$1(callback);
callback = once(callback);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependency update change

@@ -274,6 +274,7 @@ export async function scanImportsFromFiles(
loadedFiles: SnowpackSourceFile[],
config: SnowpackConfig,
): Promise<InstallTarget[]> {
await initESModuleLexer;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FredKSchott this was added because parseFileForInstallTargets() below calls parse(), and that may have been called before await init. This might not change anything, but this should help

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, safe to remove the original await initESModuleLexer; now that this is here.

I mean, the whole idea of "this only works if you call this other thing implicitly" is kind of gross, but I like this better than converting the entire function stack to async, at least for now

@drwpow drwpow merged commit 1b4ace7 into main Dec 2, 2020
@drwpow drwpow deleted the drwpow/export-map-priority branch December 2, 2020 23:17
@drwpow
Copy link
Collaborator Author

drwpow commented Dec 2, 2020

Tests passed! Merging before I break anything else 🤪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants