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: fallback to transpiled code on Node.js version <8.6.0 #160

Merged
merged 1 commit into from
May 21, 2019

Conversation

pieh
Copy link
Contributor

@pieh pieh commented May 20, 2019

As mentioned in #159 (comment), prompts crashes on Node versions between 8.0 and 8.6 because of usage of object spread operator (for example in

question[key] = typeof value === 'function' ? await value(answer, { ...answers }, question) : value;
)

This change will serve transpiled code on Node versions that don't support object spread operator

fixes #159

Copy link
Collaborator

@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.

Hey, thanks!

I know (almost) for a fact that @terkelg won't want to bring in a new library for this, especially one this big. 😬I was commenting on the issue but you were faster with the PR 😄

(I just released semiver because I often run into this same issue, and don't want to load all 62kB of semver)

The simplest fix would be to check for < 9 – the drawback is that everyone on Node 8.x still gets the Babel output, even if their version would be compatible.

I'd recommend we do something like this:

// Are we over v8.2.0?
function isOkay(tar) {
  tar = (Array.isArray(tar) ? tar : tar.split('.')).map(Number);
  let i=0, src=process.versions.node.split('.').map(Number);
  for (; i < tar.length; i++) {
    if (src[i] > tar[i]) return true;
    if (tar[i] > src[i]) return false;
  }
  return true;
}

module.exports = isOkay('8.2.0') ? require(...) : require(...);

Edit: First code snippet was wrong!

@pieh
Copy link
Contributor Author

pieh commented May 21, 2019

Thanks for quick review. Yeah, I'll make use of some helper to not pull entirity of semver

Will need to adjust your snippet as it required all major, minor and patch to be higher tho :P

@lukeed
Copy link
Collaborator

lukeed commented May 21, 2019

Yeah, I fixed it @pieh I messed up the first one. The above/current will work

Co-authored-by: lukeed <luke.edwards05@gmail.com>
@pieh
Copy link
Contributor Author

pieh commented May 21, 2019

Pushed changes, just inverted checks and added (hopefully) more descriptive name than isOkay

Once again, thanks!

Copy link
Collaborator

@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.

Looks good! 👍 Deferring to @terkelg for final verdict

@terkelg
Copy link
Owner

terkelg commented May 21, 2019

Thank you guys! Looks good – appreciate the help 💯

@terkelg terkelg merged commit e4691d7 into terkelg:master May 21, 2019
@pieh pieh deleted the topic/fix-node-lt-8-6 branch May 22, 2019 17:13
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.

Package "engines" support needs updated
3 participants