When inferring the module system (esm vs cjs) the nearest pacakge.jso… #771
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnoff
force-pushed
the
match-nearest-pkgjson
branch
3 times, most recently
from
f00395f to
f0576dd
Compare
gnoff
commented
Sep 5, 2023
Comment on lines
+387
to
+394
| var react_refresh__WEBPACK_IMPORTED_MODULE_0___namespace_cache; | ||
| __webpack_require__.r(__webpack_exports__); | ||
| /* harmony export */ __webpack_require__.d(__webpack_exports__, { | ||
| /* harmony export */ \\"default\\": () => (__WEBPACK_DEFAULT_EXPORT__) | ||
| /* harmony export */ }); | ||
| /* harmony import */ var react_refresh__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! react-refresh */ \\"../../../../node_modules/react-refresh/runtime.js\\"); | ||
|
|
||
| __webpack_require__.$Refresh$.runtime = react_refresh__WEBPACK_IMPORTED_MODULE_0__; | ||
| __webpack_require__.$Refresh$.runtime = /*#__PURE__*/ (react_refresh__WEBPACK_IMPORTED_MODULE_0___namespace_cache || (react_refresh__WEBPACK_IMPORTED_MODULE_0___namespace_cache = __webpack_require__.t(react_refresh__WEBPACK_IMPORTED_MODULE_0__, 2))); |
There was a problem hiding this comment.
previously this test caused the react-refresh runtime to be injected with a import statement putting the module into esm mode. when imported into index.js (also ESM) the exports don't require any "conversion" so the load was simple.
Now the react-refresh runtime it's being injected with the plugin (it's excluded by the webpack config /node_modules/ rule) so it stays as a cjs module. Becasue of this webpack adds this translation code because we are crossing from cjs into esm land.
gnoff
force-pushed
the
match-nearest-pkgjson
branch
2 times, most recently
from
b52a494 to
f708a99
Compare
…n should be consulted from the point of the file. The current implemetation tracks the module system by rootContext but there may be package.json files deeper than the rootContext which alter the module system which should be used. If you have a folder inside an ESM project with a package.json containing `{ type: 'commonjs' }` then all .js files in that folder and descendents should be treated like cjs. This new implementation searches for package.json files from the point of the module being imported. to avoid doing any unecessary work the result is cached along the parent path of the module context (it's folder) so that you only need to look up as far as the nearest common folder that has already had a package.json resolved.
Additionally the change removes a recently added bailout where the module system was assumed to be cjs if the LoaderContext's `this.environment` property was null. I believe this misunderstands the environment property and suggests that if it is not set then commonjs is an appropriate target. However it is possible to have a webpack config that does not produce an environment for the LoaderContext but is still an ESM project from src. Since the Loader runs before other transforms that might convert from ESM to CJS the module detection is still needed.
…Instead of feature testing with `this.environment` which was added to webpack 5 relatively recently use `typeof this.addMissingDepdencey === 'function'` which is in most (or all) webpack 5 releases but not included in webpack 4.
pmmmwh
force-pushed
the
match-nearest-pkgjson
branch
from
f708a99 to
9769038
Compare
|
Hey, sorry for ignoring this for so long - I'll give this a proper look this week. |
pmmmwh
approved these changes
Apr 27, 2024
|
No worries, glad it landed. I wanted to update the react fixtures and now I will :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When inferring the module system (esm vs cjs) the nearest pacakge.json should be consulted from the point of the file. The current implementation tracks the module system by rootContext but there may be package.json files deeper than the rootContext which alter the module system which should be used. If you have a folder inside an ESM project with a package.json containing
{ type: 'commonjs' }then all .js files in that folder and descendants should be treated like cjs. This new implementation searches for package.json files from the point of the module being imported. to avoid doing any unecessary work the result is cached along the parent path of the module context (it's folder) so that you only need to look up as far as the nearest common folder that has already had a package.json resolved.The
this.environmentfeature test is updated to usetypeof this.addMissingDependencyinstead.this.environmentwas added to LoaderContext relatively recently and many versions of webpack 5 don't support that. Regardless of whether webpack 5 is outputting esm we need to use the esm form when the package type specifically opts into esm mode so we use a feature test that is exactly on the 4/5 boundary. This function is coincidentally also needed in the implementation too.I extended the cjs and esm fixtures to have a nested package.json with the alternate type to assert that when getting the module type for a nested resource the right version is picked up.
I updated the webpack config for
getCompilationto exclude/node_modules/. Previously the loader was processing the react-refresh runtime itself but these files are not part of local users project and are not the subject of hot reloading. A realistic webpack config would exclude these files in a real project so it seems appropriate to do so here. (If there is a reason why node_modules was not excluded I can remove this, it's doesn't affect the test results)I also added calls to
addDependencyandaddMissingDependencyto allow webpack to reload when a package.json changes or is added.