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(karma-webpack): Regression in multi-compiler mode (#390) #391

Merged

Conversation

jakub-g
Copy link
Contributor

@jakub-g jakub-g commented Jan 22, 2019

This fixes a regression introduced in [4.0.0-rc4]
See #390 for details

@jsf-clabot
Copy link

jsf-clabot commented Jan 22, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

function clone(obj) {
return Object.assign({}, obj);
}
const clone = require('lodash.clone')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better avoid using lodash.clone (and lodash.*), they are deprecated and don't updated a lot of time, many of them contain security problems

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you prefer to revert to full lodash then? (or have a better suggestion?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use full lodash (if you use lodash in other places in code) or you can use https://github.com/TehShrike/deepmerge

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only place lodash would be used.
I am not even sure why we're cloning the webpack config in the first place since it's never been a deep clone, and we're mutating its attributes either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For info, cloning was added in #48 but as you mentioned, it's a shallow clone, so it in fact does a very poor protection for the original case. I'm thinking to use https://www.npmjs.com/package/clone-deep instead, but this is temporarily blocked until jonschlinkert/shallow-clone#3 is merged and released.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any point in cloning the webpack config?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need to clone the webpack config according to #48.
@jakub-g can you revert to using full lodash then? And use cloneDeep instead of clone

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're fine using Object.assign, can we not just do Array.isArray(obj) ? obj.map((o) => Object.assign({}, o) : Object.assign({}, obj) - instead of introducing all of lodash as a dependency?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean from what #48 says, I don't think Object.assign is enough (so we should use lodash or deepmerge), but we should try to avoid lodash in it's entirety - I'd pick deepmerge personally

@ryanclark
Copy link
Collaborator

If you're adding/removing dependencies, can you commit package-lock.json too please 🙂

@jakub-g jakub-g force-pushed the fix-multi-compiler-boot-fail branch from c44fbb1 to 22eda0c Compare May 21, 2019 07:53
@jakub-g
Copy link
Contributor Author

jakub-g commented May 21, 2019

Hello, I updated the PR (took a while, sorry for that) - can you check again?

For info, the build is failing with webpack_version=next config, but this is unrelated to this PR (it works with webpack_version=latest, and master alredy fails with next)

There's some unwanted diff in package-lock.json probably due to mismatch of npm versions (each version of npm generates a different lockfile, which is quite annoying...)

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 this pull request may close these issues.

5 participants