Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Update package.json engines field #509

Merged
merged 1 commit into from
May 5, 2017
Merged

Update package.json engines field #509

merged 1 commit into from
May 5, 2017

Conversation

sebmarkbage
Copy link
Contributor

yarn warns about this.

yarn warns about this.
@NTillmann
Copy link
Contributor

We had some issues with a particular node.js version > 7.0.0 --- somehow we managed to trigger a JIT crash! So this needs some testing with all our workloads.

@sebmarkbage
Copy link
Contributor Author

Interesting. The node environment that I have been building out shims for is 7! So it won't work with 6 atm. :)

@hermanventer
Copy link
Contributor

Can you run "npm run validate" on your system without any issue? If so, can you tell us which version of Node you are using to run it?

@sebmarkbage
Copy link
Contributor Author

I can confirm that I'm hitting this bug with v7.9.0.
screen shot 2017-05-04 at 3 24 27 pm

@sebmarkbage
Copy link
Contributor Author

The only place I can repro it is in tests though. It seems like a bug in contextify (the "vm" module used by the test runner).

var vm = require('vm');
let script = new vm.Script(`
  var global = this; var self = this;
  Object.defineProperty(global, "p", {
    configurable: true,
    enumerable: true,
    get: function () { throw new Error("42"); }
  });
`, { cachedDataProduced: false });
script.runInNewContext({});

If that's the only case I think we should expand the allowed engine range so that people can use the npm package outside of contributing to the project itself (i.e. running tests). While we try to fix the test.

@sebmarkbage
Copy link
Contributor Author

sebmarkbage commented May 5, 2017

So what happens is that contextify tries to copy all the properties over and one of them throws which messes up the native side. I think this is covered by the bug nodejs/node#6283

https://github.com/nodejs/node/blob/v7.10.0/src/node_contextify.cc#L96-L117

@deltaidea deltaidea mentioned this pull request May 5, 2017
@sebmarkbage
Copy link
Contributor Author

sebmarkbage commented May 5, 2017

I have been getting some additional timeouts in Test262 on this version. Let me know if we'll need to bump any timeout values.

@sebmarkbage sebmarkbage merged commit 26caa8f into master May 5, 2017
@hermanventer
Copy link
Contributor

I find the timeouts to be non deterministic on my setup. Perhaps not too surprising. The time-out setting for the CI is a bit more generous. I'm OK with leaving things as is as long as CI does not get to be flaky.

@hermanventer hermanventer deleted the engine-version branch May 5, 2017 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants