-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 Node 4 compat + add js_engines=[NODE_FS] #6098
Conversation
// Node.js < 4.5 compatibility: Buffer.from does not support ArrayBuffer | ||
// Buffer.from before 4.5 was just a method inherited from Uint8Array | ||
// Buffer.alloc has been added with Buffer.from together, so check it instead | ||
return Buffer.alloc ? Buffer.from(arrayBuffer) : new Buffer(arrayBuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this would be a good candidate for introducing what I was talking about in #5554 with respect to minimum targeted versions: perhaps we could do a link time -s MINIMUM_NODEJS_VERSION=x.y.z
, where
-s MINIMUM_NODEJS_VERSION=-1
would mean generating code that does not support running in Node.js,
-s MINIMUM_NODEJS_VERSION=0
would mean generating code that runs with all emulation to support oldest node.js version that we can support, and
-s MINIMUM_NODEJS_VERSION=x.y.z
would say that we want to generate code that works at least on Node.js version x.y.z.
Then we could gate these kinds of backwards compatibility emulation features on #if MINIMUM_NODEJS_VERSION < VERSION(4,5,0)
, and users could choose their target Node.js version to pick up the set of emulation that they need.
The advantage would be that this would be a self-documenting way to say "this thing is needed if Node.js < 4.5", and it would allow a good generic way to naturally deprecate these types of emulation features already the moment they are introduced, and users who want to optimize for absolutely minimal code size can choose their targeted version tightly.
This is just a few code lines of change in this PR so code size impact is not too big, but I wonder if this would be a good moment to introduce this, if people like this kind of model would be good? We could have similar MINIMUM_SPIDERMONKEY_VERSION
and MINIMUM_FIREFOX_VERSION
and MINIMUM_CHROME_VERSION
etc. to gate features that nicely fall to being browser/shell version dependent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually given that this fixes the bots to be green, we probably should just defer the above types of changes further, but do them as followups. This looks good to merge to me, but one Travis CI build is still in progress, so let's merge once that's finished through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Node 4 Buffers are instances of Uint8Array, so could you just create and pass a Uint8Array to the fs functions? That should be future proof.
@juj If we decide to go with Babel, we would be able to pass in a list of browser and Node versions, and have it generate lowest common denominator code. (And IMO that would be a big advantage of going with Babel.) (Though a lot of the stuff we'd be wanting to have alternatives for would be stuff you'd polyfill rather than language features, and I'm less sure how Babel works with that...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so could you just create and pass a Uint8Array to the fs functions?
That would be great but it can be done only on Node.js 7+. Sad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how many users would want features like MINIMUM_[NODEJS|FIREFOX|CHROME]_VERSION
? I would guess that a simple feature like LEGACY_VM_SUPPORT
would be simpler and good enough, as I think people either support only modern environments (say, within the last year) or they also support very far back (IE11), and the code size benefits of fiddling with specific versions is not going to be very large. Also, it would be easier for maintenance for us to not have to have a lot of different flags.
I could be wrong, though, we could do a survey to find out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An issue with LEGACY_VM_SUPPORT
might be that someone's legacy can be at a different perspective from someone else. Being able to drop all generated code, say, for running in Node.js at all might be a substantial code size win.
I do agree that the number of flags will be more, and I don't think that we would want to eagerly go and gate each and every feature on specific versions of each, but that this could be a good generic structure to gate things on that whenever these types of things come up, that one would have an existing machinery to latch the #ifdefs onto. A MINIMUM_NODEJS_VERSION < 4.5
would read better than LEGACY_VM_SUPPORT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being able to drop all generated code, say, for running in Node.js at all might be a substantial code size win.
That's different than a minimum node version, isn't it? Or are you saying that if the user sets a minimum node version of "infinity" or some other special value it would remove all node support, as a special case?
I do agree it would be nice to let builds have specific targets ("this build is only for the web" or "this build is only for node"), it's specific versions that seems excessive to me. But again, maybe I'm wrong, we could do a survey to see.
Thanks! |
Hopefully this will fix CI-uncaught errors. cc: @kripken and @juj for 39275e8.