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

test(next): add tests for Node-like hashbang support #27906

Merged
merged 14 commits into from
Aug 11, 2021

Conversation

ctjlewis
Copy link
Contributor

@ctjlewis ctjlewis commented Aug 9, 2021

The CLI would break for projects that import from an entry-point with a shebang, though they would be valid Node programs. This PR adds a Webpack loader which removes first-line shebangs from imports to prevent this error.

This approach is safe because emitted bundles will never need a shebang, and even if they did, the line could just be prepended, and you would only ever want one—thus, stripping shebangs from each resolved module is the way to go.

Hashbang logic was added directly to Webpack by @sokra and has been merged into canary. This PR now adds tests for Node-like support of first-line, Unix-style hashbangs, which should prevent regression from the fix for #27806.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ctjlewis
Copy link
Contributor Author

ctjlewis commented Aug 9, 2021

Any thoughts on how to address the React Refresh issue as far as the regression test + MJS interop conflict goes?

If the change to that runtime is reverted, the .mjs file will fail to load with the following error:

image

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ctjlewis
Copy link
Contributor Author

Even removing the typeof module !== 'undefined' check from the React Refresh logic, tests related to React Refresh are failing.

@ctjlewis
Copy link
Contributor Author

When Webpack support for hashbangs is merged, this PR will become redundant and can be closed:

webpack/webpack#13961

@sokra sokra mentioned this pull request Aug 10, 2021
@styfle
Copy link
Member

styfle commented Aug 10, 2021

Thanks for this PR!

Closing in favor of #27929 as you mentioned

@styfle styfle closed this Aug 10, 2021
@styfle
Copy link
Member

styfle commented Aug 10, 2021

Actually, lets reopen this and turn it into just tests

@styfle styfle reopened this Aug 10, 2021
@ctjlewis
Copy link
Contributor Author

ctjlewis commented Aug 10, 2021

Good idea @styfle - did you see my notes about the .mjs extension conflicting with React Refresh? I force-pushed up with commented versions of the tests. I was only able to work around by adding a typeof module !== 'undefined' check to that runtime, but it broke tests.

@styfle styfle requested a review from sokra August 10, 2021 18:47
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
* update to webpack 5.50.0

* feat: use shebang loader shim for now

* chore: snapshot formatting change

* chore: remove unneeded SSR test

* chore: test all file extensions

* chore: remove shebang loader from Webpack config

* chore: revert unnecessary changes

* Update test/integration/hashbang/test/index.test.js

Co-authored-by: Steven <steven@ceriously.com>

* chore: revert changes to yarn.lock

Co-authored-by: Tobias Koppers <tobias.koppers@googlemail.com>
Co-authored-by: Steven <steven@ceriously.com>
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants