Skip to content

Conversation

@reznord
Copy link
Member

@reznord reznord commented Jul 1, 2017

Fixes #169

@reznord reznord requested review from developit and rkostrzewski July 1, 2017 17:55
@reznord
Copy link
Member Author

reznord commented Jul 1, 2017

Travis checks are failing because lib/lib/check-engine.js is not available.

If we update the build on travis, that would fix the tests.

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Nice @reznord! Thanks for all this 😄

However, how do you feel about going the much simpler route, something like this:

const pkg = require('../../package');

const version = parseFloat( process.version.substr(1) );
const minimum = parseFloat( pkg.engines.node.match(/\d+/g).join('.') );
// ^^ or just hard-code?

if (version >= minimum) {
   return true;
}

// uh oh... log & exit

@developit
Copy link
Member

Either way process.version seems super nice here, since it makes the whole check synchronous.

@lukeed
Copy link
Member

lukeed commented Jul 1, 2017

@developit Are you opposed to moving this check-engine.js file to the root & adding it to the NPM files?

This way, we can let the tests run without doing any fancy side stepping. Currently, this will be fine with downloads since a lib/ will already be built.

@developit
Copy link
Member

Yup, I'm fine with that. Could almost be a node -e in scripts, but the root is good enough.

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Okay then, sorry @reznord 😆 😭 few changes:

  • Move lib/check-engine.js to root (maybe as version.js or check.js?)

  • Add the new root file to "files" in package.json

  • Update the paths in scripts & src/index.js

Thank you 🙏

I can take care of this too if you don't have time.

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Thanks~!

package.json Outdated
"lint": "eslint src tests",
"pretest": "npm run -s build",
"postinstall": "node lib/lib/check-engine.js",
"postinstall": "babel-node check.js",
Copy link
Collaborator

@thangngoc89 thangngoc89 Jul 1, 2017

Choose a reason for hiding this comment

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

Please don't do this. babel-node is in devDeps so it would break user install. How about rewrite check.js with CommonJS modules ?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right. Forgot not all babel pkgs are normal deps.

Let's rewrite to ES5 and just use node.

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Perfecto

@thangngoc89
Copy link
Collaborator

@reznord Your commit message make me laugh :D

check.js (es5 version)

And you're using template literal

@reznord
Copy link
Member Author

reznord commented Jul 1, 2017

Your commit message make me laugh :D

Bad time for a commit I'd say :P (2AM) anyways, its all sorted :P

@reznord reznord merged commit 01f6329 into preactjs:master Jul 1, 2017
@lukeed
Copy link
Member

lukeed commented Jul 1, 2017

Template literals ftw! 🎉

@rkostrzewski rkostrzewski added this to the 1.4.0 milestone Jul 5, 2017
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