Fix imports with a query part #603
Conversation
With the new `allowAppImports` feature, apps can import resources that are handledwith custom webpack loaders. Some can allow you to customize their behavior by passing query params, like `import image from './images/image.jpg?size=1024`. This PR is fixing two related issues: * make sure that an `allowAppImports` glob like `'images/**/*.jpg'` would also match for imports that include query params, by stripping the query part before trying to match the import to the glob * query params could technically also include quotes. These already got escaped when building the app-chunk that has the AMD-shims, but the regex to replace `EAI_DISCOVERED_EXTERNALS` was not taking their possible existance into account correctly.
simonihmig
force-pushed
the
fix-imports-with-query
branch
from
d14ae9a to
b51bc86
Compare
| function moduleToId(moduleSpecifier: string) { | ||
| let id = moduleSpecifier; | ||
|
|
||
| // if the module contains characters that need to be escaped, then map this to a hash instead, so we can easily replace this later |
There was a problem hiding this comment.
This is used for the argument to EAI_DISCOVERED_EXTERNALS(). When we have quotes in the query part of an import, mapping this part back to a module inside addDiscoveredExternals can become a bit more involved, because not only would we have to unescape the changes from js-string-escape, but in the case of devtool: 'eval' we would even end up with double-escaped quotes (' --js-string-escape--> \' --webpack-eval--> \\\').
So instead of trying to map this back to the original module specifier, I was just using something here that can serve as an identifier to the module, and which we can map back to it. In this case a md5 hash. Which has the benefit that we won't run into this kind of escaping/unescaping hell...
|
I haven't yet looked into adding tests for this. I guess for covering imports of non-JS files, including query params, we would need to add a simple webpack loader in a scenario test? Like on that returns the query as the generated module's export, so we can assert that the loader is correctly invoked and the query stuff works? Should that be a separate scenario? Or any other suggestions? |
|
Btw, I worked on these changes as part of making simonihmig/responsive-image#442 work with ember-auto-import, and it seems this is indeed the final missing piece, as the things that worked under Embroider are now also passing (locally) under eai! 🎉 |
|
This is looking reasonable. For tests, I think you can extend these where we test allowAppImports in general. Adding a loader would be OK, but it also might be enough to use built-in webpack features like Asset Modules. |
With the new
allowAppImportsfeature, apps can import resources that are handled with custom webpack loaders. Some can allow you to customize their behavior by passing query params, likeimport image from './images/image.jpg?size=1024.This PR is fixing two related issues:
allowAppImportsglob like'images/**/*.jpg'would also match for imports that include query params, by stripping the query part before trying to match the import to the globEAI_DISCOVERED_EXTERNALSwas not taking their possible existence into account correctly, leavingEAI_DISCOVERED_EXTERNALSaround in the final build.