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(node-resolve): resolve hash in path #588

Merged
merged 2 commits into from
Oct 11, 2020

Conversation

chengcyber
Copy link
Contributor

@chengcyber chengcyber commented Sep 27, 2020

Rollup Plugin Name: @rollup/plugin-node-resolve

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:

Description

This PR is fixing #587

import test from 'test#foo'

treats as hash usage

import test from 'test/#/foo'

treats hash as part of package path

@LarsDenBakker
Copy link
Contributor

This creates a bit of a special case, for instance it would not fix it for test/#foo/bar but I think at least it solves the immediate problem people are having with this. So 👍 from me

@lukastaegert
Copy link
Member

I agree this is definitely an improvement and could be released as a partial bug fix.

But looking for a long term fix, why are we stripping BOTH # and ?? Because the core of the problem is that while ? is not a valid character in file names anyway and treating it as a suffix should be safe, # is not. So why are we not fixing it by removing the special handling of # ?

I think it would still solve the original issue #486 in so far as it leaves a way to pass on information, just not via hashes. Thoughts @LarsDenBakker?

@LarsDenBakker
Copy link
Contributor

I agree, ? Is the primary use case so we could leave out hashes.

@lukastaegert
Copy link
Member

@chengcyber Would you be willing to modify this PR to remove the special handling for hashes altogether? Of course this would make this a breaking change, but I think this should be worth it in the long run.

chengcyber added a commit to chengcyber/plugins that referenced this pull request Oct 9, 2020
@chengcyber chengcyber force-pushed the fix-node-resolve-hash branch from 362a0c7 to 18acd47 Compare October 9, 2020 09:32
@chengcyber
Copy link
Contributor Author

@lukastaegert, I remove the special handling for hashes in the latest commit, please kindly take another look.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Thanks, can be merged

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.

4 participants