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]] Preserve functionality in "legacy" Node.js #3215

Merged
merged 1 commit into from Nov 21, 2017

Conversation

jugglinmike
Copy link
Member

@rwaldron In gh-3209 and gh-3210, you reported that JSHint is now failing its
own tests when run with the 0.10 and 0.12 releases of Node.js. You were right
that the error originates from a nested dependency: ironically, an npm module
named boom. My initial plan was to work with the maintainers of that module
to revert the change in a patch release and re-apply it in a new major version.

That turned out to be unnecessary because boom released a new major version
when it dropped support for legacy versions of Node.js. The real issue is how
that module has been consumed elsewhere.

The module hawk (which depends on boom) likewise incremented its major
version number in recognition of the breaking change. Unfortunately, the
request module (which depends on hawk) did not follow
suit
. And since JSHint depends
on request only transitively (through the phantom module--used to run the
tests in a browser-like environment), we cannot shield ourselves from the
breaking change through this project's package.json file.

The simplest solution would be to drop support for Node.js versions 0.10 and
0.12. I'd be open to this, but only after announcing our intention, finalizing
our pending bug fixes and feature additions, publishing one last minor version,
and then publishing a new major version. But in the mean time, we would still
have a responsibility to verify the correctness of the library in those
environments.

This patch fixes the tests in 0.10 and 0.12. We're lucky because we only need
to run PhantomJS for one build, and the version of Node.js we use to do that is
inconsequential. If we move forward with this solution, we will be saying, "We
no longer support contributing to JSHint using legacy versions of Node.js."
This isn't ideal, but it's not a breaking change.


A recent minor release of the npm module request has introduced
breaking changes for Node.js versions 0.10 and 0.12. Because JSHint
depends on this module transitively through one of its
devDependencies, this interferes with the project's ability to verify
its own correctness in the effected environments.

Allow JSHint to be installed in legacy environments by specifying the
effected dependencies as "optional." These dependencies are only
required by tests for the PhantomJS platform, and those tests can be
orchestrated from a single actively-maintained version of Node.js
without effecting coverage.

A recent minor release of the npm module `request` has introduced
breaking changes for Node.js versions 0.10 and 0.12. Because JSHint
depends on this module transitively through one of its
`devDependencies`, this interferes with the project's ability to verify
its own correctness in the effected environments.

Allow JSHint to be installed in legacy environments by specifying the
effected dependencies as "optional." These dependencies are only
required by tests for the PhantomJS platform, and those tests can be
orchestrated from a single actively-maintained version of Node.js
without effecting coverage.
@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Coverage remained the same at 98.743% when pulling f8edd41 on jugglinmike:fix-legacy-node into a11d631 on jshint:master.

@rwaldron
Copy link
Member

The simplest solution would be to drop support for Node.js versions 0.10 and
0.12. I'd be open to this, but only after announcing our intention, finalizing
our pending bug fixes and feature additions, publishing one last minor version,
and then publishing a new major version. But in the mean time, we would still
have a responsibility to verify the correctness of the library in those
environments.

This sounds good to me.

Unfortunately, the strategy used in this PR is failing in appveyor: https://ci.appveyor.com/project/jshint/jshint/build/1457/job/t4bnpwqry1jjo03n I believe we need to modify appveyor.yml by removing fast_finish: true

@jugglinmike
Copy link
Member Author

This patch has the desired effect in the Windows build, as evidenced by the
output:

npm WARN optional dep failed, continuing phantomjs-prebuilt@2.1.16

It's the text that immediately follows which describes the failure:

npm ERR! Windows_NT 6.3.9600
npm ERR! argv "C:\\Program Files (x86)\\nodejs\\\\node.exe" "C:\\Program Files (x86)\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "install"
npm ERR! node v0.10.48
npm ERR! npm  v2.15.1
npm ERR! path C:\projects\jshint\node_modules\jscs\node_modules\.bin\esparse
npm ERR! code ENOENT
npm ERR! errno 34
npm ERR! enoent ENOENT, chmod 'C:\projects\jshint\node_modules\jscs\node_modules\.bin\esparse'
npm ERR! enoent This is most likely not a problem with npm itself
npm ERR! enoent and is related to npm not being able to find a file.
npm ERR! enoent 
Command exited with code 34

I'm not sure how to interpret this. Somehow, a dependency of JSCS is not
available. Not sure why that would be the case, especially considering that it
is present when run in Node 0.10 in GNU/Linux. As a first guess, it could be a
transitory network-related problem. I'll just try manually re-starting the
build...

@jugglinmike
Copy link
Member Author

Well, would you look at that :)

@rwaldron
Copy link
Member

npm WARN optional dep failed, continuing phantomjs-prebuilt@2.1.16

My eyes and brain read right by... Sorry about that. Thanks for the follow up. This is 👍

@martinpitt
Copy link

This is really awkward.. this now pulls in phantomjs and phantomjs-prebuilt into projects that use jshint, withouth warning. These are huge and unnecessary dependencies! This only landed 3 days ago in 2.9.6 (2.9.5 is fine), so I only noticed this now. Can this please be reverted? Or want me to file a fresh issue about it?

npm install --no-optional really isn't an option, as this would apply to all of our package's dependencies. I don't see a way of blacklisting phantom, or avoiding optional dependencies just for jshintt.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Aug 3, 2018
2.9.6 pulls in phantomjs due to a
[bad hack](jshint/jshint#3215). This is a huge
and unnecessary `npm install` slow down, so avoid this by staying at
2.9.5 for now.

Reported at jshint/jshint#3318
martinpitt added a commit to cockpit-project/cockpit that referenced this pull request Aug 3, 2018
2.9.6 pulls in phantomjs due to a
[bad hack](jshint/jshint#3215). This is a huge
and unnecessary `npm install` slow down, so avoid this by staying at
2.9.5 for now.

Reported at jshint/jshint#3318

Closes #9787
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.

None yet

4 participants