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 require used in virtualModuleModernEntry template #17108

Closed
wants to merge 1 commit into from
Closed

Fix require used in virtualModuleModernEntry template #17108

wants to merge 1 commit into from

Conversation

alexgrozav
Copy link

Issue: #14877

When package.json contains { "type": "module" }, the build fails with Uncaught ReferenceError: require is not defined.

What I did

Used async/await to import all configuration files in bulk, maintaining the same functionality.

We'd still need to ensure that all the imported files have their extension specified (.js).

When **package.json** contains `{ "type": "module" }`, the build fails with `Uncaught ReferenceError: require is not defined`.

Fixes #14877

We'd still need to ensure that all the imported files have their extension specified (`.js`).
@nx-cloud
Copy link

nx-cloud bot commented Jan 2, 2022

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit cde0867.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@tmeasday
Copy link
Member

tmeasday commented Jan 7, 2022

@alexgrozav sorry about the delays here, I'm just back from leave and things have been busy. Thank you for this and I really want to take a closer look.

@alexgrozav
Copy link
Author

@tmeasday No worries. Hope it helps and works as expected. :)

@tmeasday
Copy link
Member

tmeasday commented Jan 12, 2022

Hmm, interesting solution. There is an almost-merged alternative here: #16727

This PR is in line with what the Vite builder does (async getProjectAnnotations() due to the use of import()). So there is a consistency argument towards doing things this way.

It does (unnecessarily I guess) change the behaviour of SB though -- now it needs to go and fetch the project annotations on bootup (every time) rather than having them in the initial bundle. That seems unnecessary to get round a technical issue, so I think I will close this in favour of the other PR, but keep this in the back pocket in case that one doesn't work out! Thanks @alexgrozav!

@tmeasday tmeasday closed this Jan 12, 2022
@alexgrozav
Copy link
Author

Perfectly understandable. Better to have the two builders aligned. Cheers! :)

@tmeasday
Copy link
Member

Oh! Actually I think maybe my comment above was ambiguous and it is the other way round :)

@Andarist
Copy link
Contributor

@tmeasday despite the #16727 being shipped already - I couldn't make it work with my fairly easy setup and this patch actually makes things work for me

@tmeasday
Copy link
Member

@Andarist do you have a reproduction? We could reconsider doing this PR.

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

Successfully merging this pull request may close these issues.

None yet

5 participants