install: better win, linux install experience #163
Conversation
effectively, install a 'noop' version on non-darwin
platforms. dependants will still get an Error thrown
when attempting to require('fsevents') at runtime,
indicating fsevents is not functional on current
platform, and to alter behaviour as done previous to
this commit.
|
Once caveat is that, at least for windows, pre-built binaries should be made, as it's not the default build tools exist on windows. |
effectively, install a 'noop' version on non-darwin
platforms. dependants will still get an Error thrown
when attempting to require('fsevents') at runtime,
indicating fsevents is not functional on current
platform, and to alter behaviour as done previous to
this commit.
|
@zaggino @MarshallOfSound would appreciate if you could evaluate this with the electron-rebuild context. |
Will this happen with existing config if we set the package up in appveyor? I guess we can try it and find out before accepting this. |
|
I can run this through the
In general this seems like a reasonably large risk for little reward. I know for instance that the old This module gets TLDR: I don't think a module should change how it works purely to hide a log in a package manager. IMO the package manager should change to allow the log to be suppressed. But hey, just my opinion 😆 This is also obscuring the fact the module doesn't work on windows / linux from consumers. The warning is there for a reason, attempting to hide it will do more harm than good. |
|
@MarshallOfSound it may just be a simple log message to you, but that warning causes a lot of pain for a lot of people (see npm/npm#11632, for example). Would appreciate your participation in trying to find a solution that provides a smoother experience for everyone. As to your semver point, we'll evaluate it once we have what we think is the solution, and the possibility of doing this only with a major bump is on the table. |
|
The other situation this could significantly help and that comes up often is shrinkwrap. |
|
@MarshallOfSound This module will now be used on windows and linux platforms, specifically to get rid of a constant warning that is generated. This constant warning creates a great deal of noise, and diminishes the value and detection of actual, real warnings that should be addressed. This isn't really a breaking change to the modules actual behavior, which the version number should represent, but a change related to distribution of the module, which isn't really versioned. While the module has many dependents, electron is special in that it rebuilds things, which most dependents won't be doing. The size of the source and binary is small enough to be inconsequential when installed on non-darwin systems, and for windows, as was mentioned earlier, a prebuilt binary can be made so no build tools are required there. It's not an ideal solution, but it is a solution that should keep every one happy and make some things better, and should be easily done so as not to fundamentally prevent the ecosystem from business-as-usual. Your support in this is greatly appreciated. |
|
I don't have much to say on top of what @MarshallOfSound said, but I too don't believe that |
It has been. For years. And that has been my response for a long time when this concern was raised. But I'm willing to set aside some degree of notions about what should be when opportunities exist within my/our control to solve problems. Sometimes that means trying to find compromise between the perspectives of opposing viewpoints. I remain interested to see if a solution can be found that achieves @papercuptech's goals (and, by extension, the many other users who have similar issues and concerns) without breaking or significantly impacting usage within the electron ecosystem. @papercuptech appears motivated enough to refine the implementation to avoid causing problems for your use-case, so if you can give pragmatic feedback about how it actually impacts your experience with having fsevents as a dependency we can discuss how best to proceed from there. |
Who are we to decide what the end user should and shouldn't be notified about. The warning is there for a reason and just because a portion of consumers understand the warning, expect the warning and don't want to see the warning doesn't mean that others don't still need to see it.
I fundamentally disagree here, let's use an alternate example where you have a JS module that ships ES5 code, the functionality doesn't change but we start shipping ES6 code. The functionality of the shipped module is the same, but the shipping method has changed causing it to break for some users. This is a breaking change and should be semver'ed as such.
You have to consider the Electron case where the binary will be shipped to end-users pointlessly where every single KB of the packaged app counts, in development this stray binary isn't too bad, in prod when deployed, it's just an annoyance. The prebuilt binary will only be for
It really isn't an idea solution 😢 But I spent a bit of time on this and played with a few ideas that handle all problems for everyone involved. Here is a rough idea of what I think we should do.
This would replicate the existing situation to the point of it being non-breaking I believe, I would have to do some testing with an actual implementation but I believe it would be the safest way to achieve all goals. |
|
Having an install script call
|
This is purely to absolutely minimize the size of a module install on a machine that doesn't actually use the module. In the best world all I would want in the module folder is the package json and a minimal JS file that just throws an error 😄 |
for non-darwin, on install delete 'binding.gyp' rather than call node-pre-gyp to prevent electron from trying to build, and have fsevents.js throw error when require()d.
|
@MarshallOfSound Nothing will be built on a non-darwin machine because the install script deletes |
|
@papercuptech Not just no binaries, all that code shouldn't be there either. If the module is a noop all those files are dead and should be wiped 👍 |
|
@MarshallOfSound By reasonable. There's lots of module code and even whole modules that get installed and never actually called. That's one reason why everyone is looking forward to ES6 imports as it enables tree-shaking. |
|
I have to focus on the Electron case where the node_modules folder gets shipped with the app. There is no tree shaking, especially not for potentially native department. I think it's perfectly reasonable to expect the experience to change as little as possible |
|
@MarshallOfSound That was my point. You're node_modules most likely contains js code that is never used by the app, so why you're singling out @es128 I don't intend to add install logic to remove source files, as it's just unnecessary in the bigger picture. FYI, this PR was updated to remove |
I'm not singling it out to "remove" code, I'm addressing the concern that you are adding code to the node_modules folder for no purpose. If I had infinite time I would go around all NPM modules and get them to clean up unused deps / code but I can't. I'm here now and this change in it's current state will add source files to the node_modules folder which don't need to be there.
See above as to why that attitude is invalid. I don't want to get into a fight on the validity of this change but saying "I don't see the reason" doesn't mean there isn't a reason... |
|
Its not necessary to address the issues effectively and reasonably. If you're that passionate about it, you should make a PR. |
|
@papercuptech The attitude that I should make a PR to fix a flaw I pointed out in your PR is wrong on so many levels... My only concern is ensuring that the experience for all end users is not adversely affected in any way by this change. If you don't want to meet that criteria that's on you, but my concerns have been raised and a solution was proposed. If this ever gets raised in the future it will be clear you disregarded this issue / adverse affects of this PR for no reason other than you don't consider it "necessary". I would gladly make a PR if I actually had a vested interest in fixing the original "issue" that you have, however I do not have such an interest. I am merely engaging this conversation in an attempt to ensure that by making this change you do not impact anyone or any project in any potentially negative way. TLDR: If you wish to ignore my feedback that is 100% on you and any issues that come through on my end will be directed straight here. It's a simple change to delete source files, why not do it and save us all a lot of potential hassle.... |
|
To reduce potential tension here, let me just say this. Do what you want, I have made my points, just be aware that this can and will still cause problems. |
|
The code is about ~10k? A small electron app is ~30MB?. So the 0.03% increase in size is the problem you're referring to? We use electron apps, and if a 0.03% increase in size is the cost of stopping the warning this PR addresses, not a problem for me at all; I'll make that trade every time. Besides, whose to say We use electron, this is not at all a problem. If it is for you, you really should make a PR. |
|
@papercuptech It's a matter of principle and consistency more than real world numbers.... As I said, do what you want, I've made my case. |
|
If my understanding of you concern, what you classify as a problem, is correct, then in principle, the responsibility of making the smallest sized node_modules folder bundle should fall to electron. Electron chose to use node, npm, and node-gyp, and how they interact, yet I don't believe they where ever intended to support the use case of smallest-sized node_modules. For consistency, electron being the one demanding small node_modules, it should do things like sweep it and delete all *.cc files, and probably a ton of other things. But that's just in principle and for consistency. Unfortunately, many of us live in a world of real numbers, and have to make compromises, like this PR is. If I'm not understanding your concern, please articulate. |
That's just wrong, Electron is a framework not a build tool, it has no knowledge of it's contents or any ability to delete / prune it.
My concern is as follows: As a matter of principle a node module should not ship files that it does not explicitly need to run, this includes things like As part of this change you are adding not only an entire module to two OS's that previously didn't get it (this I can live with). But also lumping a load of unneeded code with it. If I can't really explain more clearly than that.
But if all modules think that it adds up and as a matter of principle and best practices these files shouldn't be present on windows and linux machines. I am clearly not making progress on this discussion so as I said before, do what you want, it would be great though if you realized what I am trying to explain. 😆 |
|
I want to thank both of you for taking time to share your perspectives here. I wish the tone could have been less contentious, but appreciate you doing your best to keep it civil despite your opposing viewpoints. @MarshallOfSound it's really a big stretch to call an increase in code size a regression. Semver definitely does not call for any consideration of it, and you're presenting an expectation that falls outside the norms of the npm ecosystem. Module authors are free to add code and/or dependencies, and neither of those has a direct impact on versioning decisions. That said, I am sympathetic to your concern. There is a much bigger code weight problem with the node-pre-gyp bundled dependency than with the C code this discussion has been focused on so far. @papercuptech you're asking for consideration regarding your issue by those who consider it be no big deal and not fsevents' problem... you could perhaps stand to reciprocate a greater degree of consideration regarding their concerns that have been raised. Why take such a hard stand against deleting additional unneeded files? We're here because the status quo of node/npm is lacking in various ways, and I'm in the precarious position in this case of having to try to decide which tradeoffs are justified. I'm inclined to focus on practical solutions while setting aside matters of pure principle. I'm curious to find out what the outcome would be if the install script on non-darwin systems not only deleted the unneeded native code, but also ran |
|
@es128 You're the gate keeper. If you say things must be deleted before you'll accept the PR, then I will make it so. Anything else you require? |
|
It's an experiment, I haven't made final decisions yet about how it needs to be. What I'm looking for is if we do strip out what we can, including dumping the bundled dependencies, can we find a way to break within a reasonable use case. For instance, does it cause the |
|
Actually, I have other things to be doing. This PR is a viable solution, albeit a definite compromise, and addresses a problem better solved by npm and the like. If you guys want to go chase principle past practicality, I'll get out of your way. |
|
@papercuptech totally reasonable. Thank you for your contribution so far. I will pick it up and try out these ideas. |
|
@es128 any progress? |
|
@fengmk2 only that I've come to the point with it where I think that landing this would call for a major release. It's not strictly called for by semver definitions, but the potential to unleash disruptive surprises on people makes that the only responsible way to move forward with this change. Doing it as major release and also getting it out there to have an impact for people (with chokidar also needing to bump major for the same reasons, etc) leads to a cascade of work to sort through that I'm not prepared to sign myself up for right now. Maybe eventually. I'm sympathetic to the people who have concerns about that warning message, wish we could have found an easier path toward squashing it. |
for non-darwin, on install delete 'binding.gyp' rather
than call node-pre-gyp to prevent electron from trying
to build, and have fsevents.js throw error when require()d.