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: Disable compile time evaluation of import.meta.url #897

Merged
merged 9 commits into from
Sep 30, 2022
Merged

fix: Disable compile time evaluation of import.meta.url #897

merged 9 commits into from
Sep 30, 2022

Conversation

bjoluc
Copy link
Contributor

@bjoluc bjoluc commented Mar 26, 2022

I'm affected by webpack/webpack#14445, fixed in webpack/webpack#15246, available in webpack >= v5.68.0, so I'd love to have an ncc version using webpack >= v5.68.0!

EDIT: Forgot to actually set the corresponding config option at first, added that in f19ab9c.

@bjoluc bjoluc requested review from styfle and Timer as code owners March 26, 2022 09:50
@bjoluc bjoluc changed the title Bump webpack to 5.70.0 Disable compile time evaluation of import.meta.url Mar 26, 2022
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Can you add a test to ensure this behavior is working as intended?

src/index.js Show resolved Hide resolved
@bjoluc
Copy link
Contributor Author

bjoluc commented Mar 31, 2022

I added a unit test to validate that import.meta.url is not replaced in ESM builds. If CJS builds require modules that use import.meta.url, it is not evaluated as well – which may lead to runtime errors in the CJS bundle, the counter part of #895 and a separate problem of mixed CJS-ESM/ESM-CJS builds. I'd consider not evaluating import.meta.url in CJS bundles consistent though with not evaluating __dirname in ESM builds. How to fix the whole problem is a bit out of my expertise unfortunately 😕

I think some test fixtures also need to be updated due to the new webpack version? I'm happy to leave that to you though, if that's ok 😁

@styfle styfle changed the title Disable compile time evaluation of import.meta.url fix: Disable compile time evaluation of import.meta.url Apr 1, 2022
@styfle
Copy link
Member

styfle commented Apr 8, 2022

Looks like the issue is here

ncc/test/watcher.test.js

Lines 79 to 92 in a9c07e0

return {
close: () => {
this.watchEnd();
},
pause: () => {
this.paused = true;
},
getFileTimestamps: () => {
return this.timestamps;
},
getContextTimestamps: () => {
return this.timestamps;
}
};

The returned object no longer matches the latest webpack expected type here https://github.com/webpack/webpack/blob/d3a0f8de03f26a83b4d5db3cfe177617a3801df3/types.d.ts#L12062-L12097

@aruniverse
Copy link

@bjoluc @styfle anyway to help push this pr along?

@bjoluc
Copy link
Contributor Author

bjoluc commented Sep 20, 2022

@styfle Thanks for pointing me to the watcher implementation!

I updated the CustomWatchFileSystem class according to the new one over at webpack and watcher.test.js passes again. I didn't implement the bunch of deprecated getters from the Watcher interface in favor of the generic getInfo() which seems to be enough. Not an expert here though. Maybe @guybedford can quickly skim over a4ce744 and check if anything looks wildly wrong?

yarn.lock Outdated Show resolved Hide resolved
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Great work, thanks! 🎉

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

Successfully merging this pull request may close these issues.

3 participants