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

ReferenceError: require is not defined - when using "type": "module" in package #445

Closed
ntucker opened this issue Jul 11, 2021 · 13 comments · Fixed by #492
Closed

ReferenceError: require is not defined - when using "type": "module" in package #445

ntucker opened this issue Jul 11, 2021 · 13 comments · Fixed by #492

Comments

@ntucker
Copy link

ntucker commented Jul 11, 2021

Version

0.5.0-rc.1

Description

With no "type" in package.json:

__webpack_require__.$Refresh$.runtime = __webpack_require__(/*! ../../node_modules/react-refresh/runtime.js */ "../../node_modules/react-refresh/runtime.js");

Adding "type": "module"

__webpack_require__.$Refresh$.runtime = require('/home/ntucker/src/rest-hooks/node_modules/react-refresh/runtime.js');

Perhaps this is thinking "type" will also say commonjs?

You can use @rest-hooks/[email protected] as an example if you so desire.

Related

#337

https://github.com/pmmmwh/react-refresh-webpack-plugin/blob/main/loader/utils/getModuleSystem.js

Extra info

I tried removing the package.json "exports" member, but that didn't change anything.

Reproducing repo

git clone https://github.com/coinbase/rest-hooks
cd rest-hooks
yarn install
yarn build
cd ./examples/todo-app
yarn start

This puts require() in packages/normalizr/lib/special.js where packages/normalizr/package.json has "type": "module",

@pmmmwh
Copy link
Owner

pmmmwh commented Jul 11, 2021

Sorry, can you create a repo for me to test on? Module system related issues are really hard to reproduce on my side.

There does exist logic to try inferring the current module system here, but we might have missed some cases.
A workaround would be to force ESM by using options.esModule.

@ntucker
Copy link
Author

ntucker commented Jul 12, 2021

Added section with the repo I'm trying to get this working on. I tried options.esModule = true but that messed up some other things that I think were expecting commonjs for whatever reason.

@pmmmwh
Copy link
Owner

pmmmwh commented Jul 12, 2021

The option does accept include and exclude params, maybe you can try tweaking them?
I'll take a look at the example tomorrow.

@ntucker
Copy link
Author

ntucker commented Jul 12, 2021

I tried the include/exclude a bit, but after a while gave up. It seemed to behavior like module: true with the 'default' options for include and exclude specified. Maybe if I could include/exclude for 'auto' and have another matcher for forcing a specific way. Otherwise I'm forced to find all the things that need one way or another.

@ntucker
Copy link
Author

ntucker commented Jul 12, 2021

Based on https://github.com/pmmmwh/react-refresh-webpack-plugin/blob/main/loader/utils/getModuleSystem.js it looks like maybe it's only using one packageJson globally - rather than searching upwards to find the closest packagejson based on the module's directory.

@pmmmwh
Copy link
Owner

pmmmwh commented Jul 14, 2021

Based on https://github.com/pmmmwh/react-refresh-webpack-plugin/blob/main/loader/utils/getModuleSystem.js it looks like maybe it's only using one packageJson globally - rather than searching upwards to find the closest packagejson based on the module's directory.

Yes, when that was implemented it was based on a somewhat naïve assumption (for performance mainly) that node_modules would not be passed through this plugin and that sub-directory package.json usage is rare.

I guess correctness would be better here, but it could degrade cold start performance slightly since that would mean we need to traverse all directories for all branches of the tree from rootContext.

@ntucker
Copy link
Author

ntucker commented Jul 14, 2021

Hmm what leads it to cover node_modules, babel is excluded in the webpack config here https://github.com/ntucker/anansi/blob/master/packages/webpack-config-anansi/src/base/index.js#L146

@pmmmwh
Copy link
Owner

pmmmwh commented Jul 14, 2021

node_modules as in the linking of lerna to built packages? Since that would definitely be outside of the app's root.

@ntucker
Copy link
Author

ntucker commented Jul 14, 2021

The way it's currently setup it doesn't build anything in /packages/ in webpack - you have to first build them to create their /lib that is used by exports - which is what the yarn build at top level does.

@pmmmwh
Copy link
Owner

pmmmwh commented Jul 14, 2021

The way it's currently setup it doesn't build anything in /packages/ in webpack - you have to first build them to create their /lib that is used by exports - which is what the yarn build at top level does.

Yep, but since those ../../package/something routes are not excluded from the plugin (it doesn't match the default exclude rules, which is basically node_modules), they will be processed and thus get tied to the example app's package.json with the current logic. Our loader would inject code into those built files, which should be in cjs as there is no type field for the example app, and hence breaking the build.

@pmmmwh
Copy link
Owner

pmmmwh commented Jul 18, 2021

Actually, while testing this I realised that there could be another way to circumvent this issue while I continue to test a new heuristic - you can specify any paths outside of the currently compiled base path to the exclude option (alongside node_modules, and I think it would fix the issue at least for your repo.

@ntucker
Copy link
Author

ntucker commented Jul 18, 2021

I played with exclude a bit but couldn't figure it out. Not sure if it was matching correctly or anything

@aaronshaf
Copy link

Still getting this issue with:

"@pmmmwh/react-refresh-webpack-plugin": "^0.5.11",

bencodeorg added a commit to code-dot-org/code-dot-org that referenced this issue Feb 28, 2024
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 a pull request may close this issue.

3 participants