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

fix unnecessary filtering of absolute aliases #2424

Merged
merged 2 commits into from Feb 23, 2021

Conversation

jonrimmer
Copy link
Contributor

Changes

As per discussion #2413, stop esinstall from ignoring aliases just because they are absolute.

Filtering out absolute aliases breaks one of the main use-cases for aliases—replacing a dependency imported by 3rd-party code with a shim, a polyfill or other locally supplied alternative. The discussion gives the concrete example of Chart.js importing Moment.js, but undoubtedly there are more.

Testing

Ran yarn test and ran the modified Snowpack against my project to confirm the change fixed the issue.

Docs

No, because this was never documented behaviour.

@vercel
Copy link

vercel bot commented Jan 21, 2021

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/7paxj3574
✅ Preview: https://snowpack-git-fork-jonrimmer-fix-alias-filtering.pikapkg.vercel.app

@FredKSchott
Copy link
Owner

I actually had the same thought when I was just in that code the other day.

The only thing to look out for is that this opens the door for a new bug in Snowpack: If I wanted to create an alias in my own application (ex: $components/* to ./src/components) then that alias now applies within all dependencies as well. Worse, because of the fact that we're pre-bundling deps, that source code would now get bundled in with the package.

Maybe that's pretty rare, if your alias is also a package name then you're probably already going to run into trouble.

But I wonder if there's a better solution than alias for this problem.

@jonrimmer
Copy link
Contributor Author

@FredKSchott Good point. Taking another look at the sources here, I notice that esinstall has a config field called importMap that seems like it might work to re-map package imports everywhere. Could it be as simple as exposing this config on the Snowpack resolve config object alongside alias?

@FredKSchott
Copy link
Owner

#2707 now brings package handling and build handling together, so this behavior now makes more sense. I think this is okay to merge and release as a part of that PR & a larger v3.1.0 release.

Thanks again for the help!

@FredKSchott FredKSchott merged commit 81d9ff8 into FredKSchott:main Feb 23, 2021
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