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(babel): strip hash and query param in extension filter #533

Merged
merged 3 commits into from Oct 28, 2020

Conversation

csr632
Copy link
Contributor

@csr632 csr632 commented Aug 9, 2020

Rollup Plugin Name: @rollup/plugin-babel

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers: Fix: #500

Description

When babel plugin is filtering module by file extensions, it should consider paths that contains hash and query param, and strip them out before doing file extension matching.

The motivation and implementation is similar to how node-resolve plugin support hash and query.

@csr632
Copy link
Contributor Author

csr632 commented Aug 9, 2020

@Andarist

@ctjlewis
Copy link
Contributor

ctjlewis commented Aug 11, 2020

Thanks @Andarist and @csr632, great work!

@shellscape
Copy link
Collaborator

@Andarist failing CI aside, where do we stand on this one?

@shellscape
Copy link
Collaborator

@csr632 could you take a look at the CI failures please?

@csr632
Copy link
Contributor Author

csr632 commented Sep 10, 2020

The CI is fixed, but we still need to decide how to handle both of these appropriately:

  • 'path/to/file#fragment'
  • 'es5-ext/array/#/concat'

Should we extract this logic into @rollup/plugin-utils? What do you think? @shellscape

@shellscape
Copy link
Collaborator

@csr632 a util in pluginutils would be reasonable. @Andarist thoughts?

This might also conflict with #588

@lukastaegert
Copy link
Member

In light of #588 I think we should NOT add special handling for # but just for ?. Reasoning is that # is a perfectly valid character in file names, so it is impossible to implement the logic in a way that does not cause conflicts, albeit in contrived examples. ? on the other hand is an invalid character for filenames anyway and special handling should therefore not cause issues.

@lukastaegert
Copy link
Member

Considering that without special handling for # this helper becomes rather short, I would for the time being vote against putting this into pluginutils as I see this causing more friction than it helps, but other opinions are welcome here.

@csr632
Copy link
Contributor Author

csr632 commented Oct 27, 2020

In light of #588 I think we should NOT add special handling for # but just for ?. Reasoning is that # is a perfectly valid character in file names, so it is impossible to implement the logic in a way that does not cause conflicts, albeit in contrived examples. ? on the other hand is an invalid character for filenames anyway and special handling should therefore not cause issues.

I just update this PR to reflect this. It is ready to be reviewed. @lukastaegert @Andarist

@shellscape shellscape merged commit 83bcdcf into rollup:master Oct 28, 2020
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.

[plugin-babel] should works with query param in path
6 participants