Skip to content

Add .nvmrc and node version checker#67

Merged
RubenVerborgh merged 3 commits intonodeSolidServer:masterfrom
bbogdanov:master
Oct 2, 2018
Merged

Add .nvmrc and node version checker#67
RubenVerborgh merged 3 commits intonodeSolidServer:masterfrom
bbogdanov:master

Conversation

@bbogdanov
Copy link
Copy Markdown

No description provided.

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 2, 2018

Coverage Status

Coverage remained the same at 64.653% when pulling e45a318 on bbogdanov:master into 54a320d on solid:master.

Copy link
Copy Markdown
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Thanks for proposing. I'm not convinced about having .nvmrc in the repository though, isn't this a developer-specific preference?

Comment thread check-engines-requirements.js Outdated
@@ -0,0 +1,13 @@
var semver = require('semver')
var packageJson = require('./package')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's use const for imports.

Comment thread check-engines-requirements.js
Comment thread .nvmrc Outdated
@RubenVerborgh
Copy link
Copy Markdown
Contributor

@bbogdanov Shall we just make the checker a hidden file and executable? Then it's all good to go.

@bbogdanov
Copy link
Copy Markdown
Author

@bbogdanov Shall we just make the checker a hidden file and executable? Then it's all good to go.

@RubenVerborgh The file is now a dot file and executable.

@bbogdanov bbogdanov closed this Oct 2, 2018
@bbogdanov bbogdanov reopened this Oct 2, 2018
@RubenVerborgh
Copy link
Copy Markdown
Contributor

Thanks! is .nvmrc still missing a newline though?

@RubenVerborgh RubenVerborgh merged commit 1d06d38 into nodeSolidServer:master Oct 2, 2018
@RubenVerborgh
Copy link
Copy Markdown
Contributor

Thanks a lot!

@bbogdanov
Copy link
Copy Markdown
Author

@RubenVerborgh I just realized the checker is running before the installation of the packages but the checker depends on one of the packages - semver. Travis seems is not using the post and pre-install scripts at the moment. Would you like me to set the script to run on postinstall? Or to make the script npm install semver && node .check-engines-requirements? I think the first approach is a better one.

@RubenVerborgh
Copy link
Copy Markdown
Contributor

Ooh yes first approach please. I'll just fix it myself in master.

@RubenVerborgh
Copy link
Copy Markdown
Contributor

Caught it in cf7d5ce, thanks.

@bbogdanov
Copy link
Copy Markdown
Author

Great, you are welcome!

@RubenVerborgh
Copy link
Copy Markdown
Contributor

The engine check caused problems when installing the package, since the script was being run on postinstall. Rather than tweaking this, I removed the script in 663e28f. (Have not seen other repositories use it.)

@bbogdanov
Copy link
Copy Markdown
Author

Could you give me more information about them/it?

@RubenVerborgh
Copy link
Copy Markdown
Contributor

So when running npm install in a repository that has solid-auth-client as one of its dependencies, the following problems would occur:

  • the script would not be found
  • semver would not be installed (because it is a devDependency)

These issues broke the release of solid-auth-client 2.2.7, which had to be retracted.

While they might be fixable (in multiple ways), I think the fragility is bigger than the utility.

@bbogdanov
Copy link
Copy Markdown
Author

I didn't have this in mind. You are right, these problems could occur. Thank you for letting me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants