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

Trigger recompilation on node module changes. #212

Closed
wants to merge 1 commit into from
Closed

Trigger recompilation on node module changes. #212

wants to merge 1 commit into from

Conversation

eanplatter
Copy link
Contributor

An Attempt at fixing #186.

This plugin offers a bit more than just recompiling when the node_modules change, it also auto installs dependencies that are referenced if they don't already exist (which may or may not be a good thing).

Personally I like the feature, but I could see it being obtrusive, I am open to solving this in a different way.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

Can you explain what

You can provide a Function to the dev to make it dynamic:

in the README means? Why would I want that, and why does the example show build dependencies when presumably the project is about runtime deps?

cc @ericclemmons

@eanplatter
Copy link
Contributor Author

It looks like you can just intercept the request, like in the example it shows a sort of whitelist of dev dependencies:

plugins: [
  new NpmInstallPlugin({
    dev: function(module, path) {
      return [
        "babel-preset-react-hmre",
        "webpack-dev-middleware",
        "webpack-hot-middleware",
      ].indexOf(module) !== -1;
    },
  }),
],

I guess to prevent devDependencies from receiving unapproved modules? Though, I don't know when that would really happen, unless you manually attempted to install something.

@ghost ghost added the CLA Signed label Jul 26, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

The lack of feedback is not very nice:

screen shot 2016-07-26 at 15 14 12

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

Yea, I’d like to get this in but the lack of progress indicator is unfortunately a blocker for me.
I’m happy to revisit this in the future if NpmInstallPlugin implements this.

@gaearon gaearon closed this Jul 26, 2016
@ghost ghost added the CLA Signed label Jul 26, 2016
@ForbesLindesay
Copy link
Contributor

I'm a bit concerned by the idea of automatic installation of dependencies. It significantly increases the risk of being exposed to malicious code via small typos.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

^^ also a very good point. I think npm install is how we want people to do it. But we need to try re-resolving deps if package.json changes.

@ForbesLindesay
Copy link
Contributor

We also may want to add something more to the docs regarding how exactly to install dependencies.

@eanplatter
Copy link
Contributor Author

Ahh, yeah I'd have to agree. It's a fancy feature but there are some definite drawbacks.

I think for this problem the best solution will to have a webpack plugin that just recompiles if the package.json or node_modules change. I'll start looking into a solution for that tonight.

1 similar comment
@eanplatter
Copy link
Contributor Author

Ahh, yeah I'd have to agree. It's a fancy feature but there are some definite drawbacks.

I think for this problem the best solution will to have a webpack plugin that just recompiles if the package.json or node_modules change. I'll start looking into a solution for that tonight.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

@eanplatter Thanks!
@ForbesLindesay Totally. We should add a section here. Let me file an issue.

@ericclemmons
Copy link
Contributor

Yea, I’d like to get this in but the lack of progress indicator is unfortunately a blocker for me.
I’m happy to revisit this in the future if NpmInstallPlugin implements this.

@gaearon Feel free to open an issue if you find something cosmetic a blocker ;)

https://github.com/ericclemmons/npm-install-webpack-plugin/issues/new?title=Dan%20doesn%27t%20like%20it

Lately, I use the defaults so that all deps save to dependencies rather than devDependencies by default. Rapid prototyping means being able to run now and have a functioning app. Moving enzyme to devDependencies is a trivial optimization for later, IMO.

But, my overall thought is that it's not this project's job to save users from using npminstall`. I've come across several libraries that have specific instructions for which version to install based on which version of React, ESLint, or even Node/NPM you're running.

To me, npm-install-webpack-plugin is DX you opt-in to like HMR because of edge-cases & "gotchas" that the user should be aware of before accepting it. Because, no matter what, there'll be some situation that won't work perfectly and that slight bump in the road ruins the otherwise smooth ride.

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.

5 participants