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

Webpack 5 does not correctly transpile .js file with "type":"module" #23029

Closed
JacobLey opened this issue Mar 13, 2021 · 7 comments · Fixed by #33637
Closed

Webpack 5 does not correctly transpile .js file with "type":"module" #23029

JacobLey opened this issue Mar 13, 2021 · 7 comments · Fixed by #33637
Labels
bug Issue was opened via the bug report template.

Comments

@JacobLey
Copy link
Contributor

JacobLey commented Mar 13, 2021

What version of Next.js are you using?

10.0.8

What version of Node.js are you using?

14.16.0

What browser are you using?

Chrome (unrelated)

What operating system are you using?

macOS

How are you deploying your application?

Custom server (unrelated)

Describe the Bug

NextJS's default configs with Webpack5 does not correctly transpile .js files under a "type":"module" package, using import/export syntax

See example repo+readme for example/details: https://github.com/JacobLey/next-webpack5-jsx-fail

High level is that the provided webpack5 + babel configuration fails to properly transpile .js. It improperly treats these modules as pure CommonJS, resulting if failures where .default should be used.

See the notable differences in the two out server files:
https://github.com/JacobLey/next-webpack5-jsx-fail/blob/main/fail/next/.next/server/pages/_app.js#L22-L27
https://github.com/JacobLey/next-webpack5-jsx-fail/blob/main/success-1/next/.next/server/pages/_app.js#L24-L29

Side note

The "type":"module" has benefits outside NextJS, but there have been times Webpack has complained about lack of "type":"module" for files that contain import/export. I would expect babel to handle this automagically with the sourceType:unambiguous setting.

I mostly bring it up because:

  1. It also seems like a bug (but not an issue enough for me to dig much deeper to consistently reproduce)
  2. It might be related, since it would also impact ability to parse/detect ESM

Expected Behavior

Webpack5 should properly parse .js files using import/export syntax within "type":"module" packages.

Considering "type":"module" enables ESM syntax like import/export if anything I would expect this to succeed over things like similar syntax in CommonJS

To Reproduce

See mini-repo to reproduce error, based on create-next-app boilerplate: https://github.com/JacobLey/next-webpack5-jsx-fail

  1. Clone repo locally
  2. npm ci
  3. npm run build:fail will fail with Class extends value #<Object> is not a constructor or null error.
  4. npm run build:success-N where N is 1-4. These highlight successful versions of the same code with minor tweaks (showing behavior can/should work). Differences are documented in README.md
@JacobLey JacobLey added the bug Issue was opened via the bug report template. label Mar 13, 2021
@JacobLey
Copy link
Contributor Author

JacobLey commented Mar 21, 2021

On some further insight, I think this bug is actually caused by how NextJS exports its packages.

Take document for example: https://github.com/vercel/next.js/blob/canary/packages/next/document.js

compare outputs of:
console.log(require('next/document.js'))
and import('next/document.js').then(console.log)

On the import the default is being overridden with a full object, instead of just Document

This is expected behavior in NodeJS https://nodejs.org/api/esm.html#esm_import_statements

@JacobLey
Copy link
Contributor Author

I don't think it is possible for CJS to actually export both a default and named exports, so values like Document should get a named export as well, so files loading it can just reference the named export instead.

e.g. import { Document } from 'next/document.js'

@JacobLey
Copy link
Contributor Author

@timneutkens would you be open to a PR adding all the default exports as a named one as well?

For all the .js files here: https://github.com/vercel/next.js/tree/canary/packages/next
(well in whatever locations those .js files point to)

e.g.

export default class Document<P = {}> extends Component<DocumentProps & P> {
...
}

export { Document };

@balazsorban44
Copy link
Member

Hi, Next.js has added native ESM support in Next 12. Please give it a try.

https://nextjs.org/blog/next-12#es-modules-support-and-url-imports

@JacobLey
Copy link
Contributor Author

@balazsorban44 I have updated the example code provided to next12.

The bug still exists.

NextJS does not support writing code as ES modules yet, it only supports importing those files from third parties.

Fingers crossed this sort of issue will be resolved by features like #33637 or via the solution I provided above

But until then I would appreciate keeping this issue open as it has not been resolved

@balazsorban44 balazsorban44 reopened this Jan 28, 2022
@kodiakhq kodiakhq bot closed this as completed in #33637 Feb 15, 2022
kodiakhq bot pushed a commit that referenced this issue Feb 15, 2022
- [x] Add failing test for development / production
- [x] Add failing test for client-side JavaScript
- [x] Write `.next/package.json` with `"type": "commonjs"
- [x] Fix issue with client-side JavaScript showing `module` is not defined

Production works after these changes. Development breaks on module not existing because of the Fast Refresh loader. Working with @sokra to add alternatives to what is being used in the loader to webpack so that it can be updated.

Fixes #23029, Fixes #24334



## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [x] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `yarn lint`
@kachkaev
Copy link
Contributor

kachkaev commented Feb 15, 2022

Happy to confirm that #33637 worked for me in a side project 🎉

I just upgraded to 12.0.11-canary.16 and then removed the .next/package.json hack. No issues with next dev, next build and next start so far, including hot-reloading!

Miraculas times, thanks a lot @timneutkens! ES Modules FTW 🚀

@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template.
Projects
None yet
3 participants