Skip to content

Comments

support query in ExportsFieldPlugin#225

Merged
sokra merged 2 commits intowebpack:masterfrom
vankop:add-tests-and-types
Jun 15, 2020
Merged

support query in ExportsFieldPlugin#225
sokra merged 2 commits intowebpack:masterfrom
vankop:add-tests-and-types

Conversation

@vankop
Copy link
Member

@vankop vankop commented Jun 9, 2020

  • add exports field test cases
  • add types export

- add exports field test cases
- add types export
@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #225 into master will decrease coverage by 0.04%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   95.17%   95.12%   -0.05%     
==========================================
  Files          37       37              
  Lines        1368     1375       +7     
==========================================
+ Hits         1302     1308       +6     
- Misses         66       67       +1     
Impacted Files Coverage Δ
lib/index.js 98.03% <ø> (ø)
lib/ExportsFieldPlugin.js 95.65% <90.90%> (-1.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf930fe...8ffab8c. Read the comment docs.

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

const remainingRequest = request.query
? (request.request === "." ? "./" : request.request) + request.query
: request.request;
if (remainingRequest === undefined) return callback();
Copy link
Member

Choose a reason for hiding this comment

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

You should move this line one up to keep the expected behavior

paths,
(p, callback) => {
const error = checkExportsFieldTarget(p);
const queryIndex = p.lastIndexOf("?");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const queryIndex = p.lastIndexOf("?");
const queryIndex = p.indexOf("?");

query string could contain ? itself

[]
]
},
//#endregion
Copy link
Member

Choose a reason for hiding this comment

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

add a test case for { "./a?b?c/": "./" } "./a?b?c/d?e?f" -> ./d?e?f

Looks weird I know^^

@webpack-bot
Copy link

@vankop Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@sokra sokra merged commit cf9cd25 into webpack:master Jun 15, 2020
@sokra
Copy link
Member

sokra commented Jun 15, 2020

Thanks

@vankop vankop deleted the add-tests-and-types branch July 20, 2020 15:27
graphite-app bot pushed a commit to oxc-project/oxc-resolver that referenced this pull request Aug 20, 2025
…ts field (#655)

Resolving `package?query#fragment` to packages with exports field is allowed in Vite to allow things like `css-package?url`, `wasm-package?init`. This was not allowed by oxc-resolver and this PR allows that.

The current behavior of oxc-resolver comes from this change in enhanced-resolve (webpack/enhanced-resolve#225). This PR adds tests that disallows `pkg?foo` but allows `pkg/nested?foo`.
This current behavior doesn't make sense to me. Both `pkg?foo` and `pkg/nested?foo` are not allowed by Node and I don't find a reason to treat them differently.

fixes vitejs/rolldown-vite#382
refs 97dfb1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants