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

Using lodash-es #1269

Closed
KidkArolis opened this issue Jun 18, 2019 · 11 comments · Fixed by #1288
Closed

Using lodash-es #1269

KidkArolis opened this issue Jun 18, 2019 · 11 comments · Fixed by #1288

Comments

@KidkArolis
Copy link
Contributor

If you configure a lodash alias in webpack:

config.resolve.alias['lodash'] = path.dirname(require.resolve('lodash-es'))

RHL fails with:

Uncaught TypeError: merge is not a function
    at updateProxyById (react-hot-loader.development.js:1089)
    at Object.register (react-hot-loader.development.js:2533)

Because const merge = require('lodash-es/merge') results in { default: merge }.
Haven't figured out how to workaround this yet.

@KidkArolis
Copy link
Contributor Author

One suggestion would be to ship an esm version of the library linked to under "module" entry.

@KidkArolis
Copy link
Contributor Author

KidkArolis commented Jun 18, 2019

For now, this workaround works:

if (!options.production) {
  config.resolve.alias['lodash/merge'] = path.dirname(require.resolve('lodash/merge'))
}
config.resolve.alias['lodash'] = path.dirname(require.resolve('lodash-es'))

@KidkArolis
Copy link
Contributor Author

Would you accept a PR that removes the use of lodash and replaces lodash/merge with something inline.

@theKashey
Copy link
Collaborator

With pleasure :)

@msudgh
Copy link

msudgh commented Jul 4, 2019

I'm wondering the following error is related to this issue:

ERROR in ./node_modules/react-hot-loader/dist/react-hot-loader.development.js
Module not found: Error: Can't resolve 'lodash/merge' in '/Users/mcrunix/w/phizog/node_modules/react-hot-loader/dist'
 @ ./node_modules/react-hot-loader/dist/react-hot-loader.development.js 1057:12-35
 @ ./node_modules/react-hot-loader/patch.js
 @ multi react-hot-loader/patch webpack-hot-middleware/client?path=http://localhost:3000/__webpack_hmr&reload=true ./app/index

Also i had test @KidkArolis workaround but didn't work.

@theKashey
Copy link
Collaborator

theKashey commented Jul 4, 2019

The best solution would be to get rid of merge. It's not actually required - the option override it was used for is now a default value.

@msudgh
Copy link

msudgh commented Jul 5, 2019

I'm not familiar with the logic of RHL proxy as the following but in case of merging objects you have spreading operator or import merge lib in es6 way import merge from 'lodash/merge.

merge({}, renderOptions, { proxy: componentOptions.get(type) || {} }, options),

@theKashey
Copy link
Collaborator

It's more about - this code is really not needed anymore.

@msudgh
Copy link

msudgh commented Jul 6, 2019

Just another point, I feel it'd be great to consider a stable tag to not release all versions on NPM. Every time i tried to run my project for a 1-2 month period i got an error which related to react-hot-loader.

theKashey added a commit that referenced this issue Jul 6, 2019
@theKashey
Copy link
Collaborator

Should be fixed in 4.12.4

@msudgh - there is no such thing as "stable" here. I always have to address compatibility issues with the rest of the ecosystem, like this one. The second part of problem is "I", not "We", which hugely affects testing scope.

@OliverJAsh
Copy link

I also just ran into this. You can fix it by scoping the webpack alias to exclude node_modules. See webpack/webpack#9690 (comment).

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.

4 participants