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

Strip query strings from imports when resolving #98

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

marcins
Copy link

@marcins marcins commented Aug 22, 2022

In some cases imports will contain query strings. The enhanced-resolver plugin for the "exports" field in package.json will fail to resolve these imports correctly. This change will strip any query strings from the import before resolving it.

The actual use case for this is that with Parcel we have created a set of plugins that will support Webpack's "magic comments" for naming async chunks. The way this plugin works is taking the chunk name and appending it to the import, e.g. import('@some/pkg?webpackChunkName=some-pkg' so it can later be accessed by a namer plugin.

RLL will fail on these imports in the rare case that the package contains an exports field, as the full string will be used to match so the query string will break things.

Added a new failing test for this - note as per the comment I was unable to get it working as a "fixture" because it doesn't appear fixtures are processed as modules, so the exports field wouldn't get used.

JakeLane
JakeLane previously approved these changes Aug 23, 2022
tomgasson
tomgasson previously approved these changes Aug 29, 2022
@mattcompiles
Copy link

So it turns out that removing query params in just the babel-plugin causes issues when attempting to do manifest lookups as the query param will still be present inside the module graph.

I've now updated the PR to strip query params in all places we reference a specifier/request.

JakeLane
JakeLane previously approved these changes Oct 23, 2022
@@ -116,3 +116,16 @@ export const buildManifest = (
{ publicPath, assets: {} }
);
};

export const removeQueryParams = (input: string) => {
Copy link
Author

Choose a reason for hiding this comment

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

If it's duplicated 3 times it should probably be extracted somewhere, but I also don't know whether there's a suitable shared dependency all the usages could import from. The function is trivial enough that it's possibly not worth the additional effort (if there's no existing common RLL package that's available everywhere)

Choose a reason for hiding this comment

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

I duplicated because there isn't a shared dep and I didn't think it was worth introducing one. In the past I've dealt with this by having internal dev deps that are compiled into the package. You need a build script capable of handing the externalisation logic though.

marcins and others added 6 commits November 14, 2022 15:15
In some cases imports will contain query strings. The enhanced-resolver
plugin for the "exports" field in package.json will fail to resolve
these imports correctly. This change will string any query strings from
the import before resolving it.
@nr-tw nr-tw force-pushed the fix-issue-with-exports-and-query-strings branch from d3cc489 to e88eca9 Compare November 14, 2022 04:15
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.

5 participants