-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
dynamic imports fix for non-webpack environments. #3414
Conversation
Oh! I didn't see this: #3414 Sorry for that. |
@@ -9,7 +9,7 @@ const TYPE_IMPORT = 'Import' | |||
|
|||
const buildImport = (args) => (template(` | |||
( | |||
typeof window === 'undefined' ? | |||
typeof require.resolveWeak !== 'function' ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment where why we use this rather a simple window
check.
It's better for someone who's reading the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the status.
@arunoda No problem. I should have added documentation in the first place. Anyhow added the requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added requested changes
@PrudviGali this looks great. |
@arunoda Cool. Thanks. |
Very nice, thanks @PrudviGali! |
Added issue description in #3345 .
Feature detection condition comes handy rather than
typeof window
because of webpack specific methodsrequire.resolveWeak
andresolve.ensure
fails in non-webpack environments.The issue was not with
window
object but withrequire.resolveWeak
andrequire.ensure
methods used in next handle-import.js file. Both these methods are specific to webpack and would not be available innode/commonjs
environment. Jest runs in Node environment and does not recognizerequire.resolveWeak
andrequire.ensure
methods.TC39 dynamic import proposal works pretty well with Jest testing. Please refer to the repo. But Nextjs implementation of dynamic imports fails in jest because of above two methods repo.
@arunoda already mentioned that should not use
process.env.NODE_ENV === 'test'
in #3405. Assuming this would be the appropriate way of handling this issue #3345 .