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

[node-resolve] Support hashes or query params in path #486

Closed
LarsDenBakker opened this issue Jul 6, 2020 · 6 comments · Fixed by #487
Closed

[node-resolve] Support hashes or query params in path #486

LarsDenBakker opened this issue Jul 6, 2020 · 6 comments · Fixed by #487

Comments

@LarsDenBakker
Copy link
Contributor

  • Rollup Plugin Name: node-resolve
  • Rollup Plugin Version: any

Feature Use Case

In the browser it's common to use hashes or query params to pass along some state. Node resolve fails to solve imports with query params or hashes.

Feature Proposal

Resolve imports with query param or hashes. For example an import such as:

import foo from 'bar/bar.js?x=y';

Should be resolved to:

import foo from '/node_modules/bar/bar.js?x=y';

I can work on a PR for this we think this feature is valueable.

@shellscape
Copy link
Collaborator

Agree this is valuable. Please do PR this!

@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Jul 6, 2020

I started working on this where node resolve would resolve the path without the query params and hashes, and then puts them back on at the end.

However this breaks further bundling because rollup will try to find a file on the file system with the query param or hash. (for example /Users/<name>/projects/my-project/node_modules/bar/index.js).

For my use case it's OK, because I'm using the plugin for single file transforms for the browser. But it I'm not sure how this should be implemented properly in the plugin. @lukastaegert do you have any insights here? What happens when you use rollup in the browser? Are imports resolved to browser imports?

@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Jul 6, 2020

Actually, it looks like rollup doesn't like query params in general: https://rollupjs.org/repl/?version=2.19.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMiUyRiolMjBEWU5BTUlDJTIwSU1QT1JUUyU1Q24lMjAlMjAlMjBSb2xsdXAlMjBzdXBwb3J0cyUyMGF1dG9tYXRpYyUyMGNodW5raW5nJTIwYW5kJTIwbGF6eS1sb2FkaW5nJTVDbiUyMCUyMCUyMHZpYSUyMGR5bmFtaWMlMjBpbXBvcnRzJTIwdXRpbGl6aW5nJTIwdGhlJTIwaW1wb3J0JTIwbWVjaGFuaXNtJTVDbiUyMCUyMCUyMG9mJTIwdGhlJTIwaG9zdCUyMHN5c3RlbS4lMjAqJTJGJTVDbmlmJTIwKGRpc3BsYXlNYXRoKSUyMCU3QiU1Q24lNUN0aW1wb3J0KCcuJTJGbWF0aHMuanMnKS50aGVuKGZ1bmN0aW9uJTIwKG1hdGhzKSUyMCU3QiU1Q24lNUN0JTVDdGNvbnNvbGUubG9nKG1hdGhzLnNxdWFyZSg1KSklM0IlNUNuJTVDdCU1Q3Rjb25zb2xlLmxvZyhtYXRocy5jdWJlKDUpKSUzQiU1Q24lNUN0JTdEKSUzQiU1Q24lN0QlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJtYXRocy5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJpbXBvcnQlMjBzcXVhcmUlMjBmcm9tJTIwJy4lMkZzcXVhcmUuanMlM0Zmb28lM0RiYXInJTNCJTVDbiU1Q25leHBvcnQlMjAlN0JkZWZhdWx0JTIwYXMlMjBzcXVhcmUlN0QlMjBmcm9tJTIwJy4lMkZzcXVhcmUuanMnJTNCJTVDbiU1Q25leHBvcnQlMjBmdW5jdGlvbiUyMGN1YmUlMjAoeCUyMCklMjAlN0IlNUNuJTVDdHJldHVybiUyMHNxdWFyZSh4KSUyMColMjB4JTNCJTVDbiU3RCUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJzcXVhcmUuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwZGVmYXVsdCUyMGZ1bmN0aW9uJTIwc3F1YXJlJTIwKCUyMHglMjApJTIwJTdCJTVDbiU1Q3RyZXR1cm4lMjB4JTIwKiUyMHglM0IlNUNuJTdEJTIyJTJDJTIyaXNFbnRyeSUyMiUzQWZhbHNlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

so maybe we need to fix this in multiple places

@lukastaegert
Copy link
Member

I like to understand this first. As you are bundling anyway, I would assume the query parameters are meant to influence the bundling process and be consumed by another plugin, is that correct?

In extension, if they are to influence the bundling process, then using different parameters needs to resolve to different modules, is that correct as well?

Then I would say vanilla Rollup supports this well enough. However, you would need to make sure you define a loader that tells Rollup how such a module is meant to be loaded as I would assume, Node does not resolve query parameters very well.

@LarsDenBakker
Copy link
Contributor Author

This in the context of tools like es-dev-server, where I'm reusing the rollup node resolve plugin. I want to resolve bare imports, but preserve the query params and hashes as they were in the original source code.

I'm looking for how this can be implemented in a way that still makes sense for regular use of the plugin. You're right that in the context of bundling with rollup a query parameter can be an instruction to another plugin which could implement a load hook. The commonjs plugin does this already.

I whipped something up in #487

@csr632
Copy link
Contributor

csr632 commented Jul 16, 2020

I have encounter this issue with rollup plugin babel. It doesn't recognize .ts extension because the module id is file.ts?query=true.

I have created an issue for plugin-babel: #500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants