GNOME Bugzilla – Bug 636652
Possibly change default js version to "default" (instead of 1.8)
Last modified: 2016-11-18 08:59:08 UTC
The big picture motiviation here is that Mozilla has definitely shown willingness to toss their "proprietary" extensions before. For example, most recently they turned off allowing webpages to access XUL. In the context of JavaScript, they removed some custom prototype handling bits. Personally, I think it quite possible, if not likely, that they may back out some of Brendan's random JS extensions like "let/yield" in "JavaScript 1.7" etc. This series of patches preserves Gjs' historical decision to use JS 1.8 by default; however, it allows consumers (e.g. gnome-shell like embedders, people just using gjs ~/myscript.js) to explictly specify a version.
Created attachment 175964 [details] [review] context: Use G_TYPE_STRV for search-path No reason to use a pointer, there's a GObject boxed for char **.
Created attachment 175965 [details] [review] Makefile-test: Fix RUN_WITH_DBUS usage
Created attachment 175966 [details] [review] context: Add a "js-version" GObject property Allow consumers to specify the JavaScript version, rather than hardcoding 1.8. The default for this patch remains 1.8 though.
Created attachment 175967 [details] [review] context: Add functions to scan a file/buffer for a JS version To more cleanly allow gjs to support multiple JS versions, add utility functions which will allow context API consumers (e.g. /usr/bin/gjs) the ability to choose the JS version based on a comment in the input.
Created attachment 175968 [details] [review] console: Detect input JS version, add --js-version argument
Created attachment 175969 [details] [review] gjs-unit: Pull in JS version from header in tests; add version to tests For now, explicitly specify all tests as being "application/javascript;version=1.8". We will back down some of the tests to default soon.
Created attachment 175970 [details] [review] Add explicit tests of JS version functionality
Review of attachment 175967 [details] [review]: ::: gjs/context.c @@ +737,3 @@ + */ +const char * +gjs_context_scan_buffer_for_js_version (const char *buffer, it's tough for me to see how the app would know what to use for maxbytes (other than, the full size of the file, which sucks). maybe "on the first or second line" should be the default / only option ? I'd say "on the first line" but I guess emacs magic or #! might be there.
Where is this bug coming from? Did upstream indicate somehow that this was likely? Adding ability to downgrade makes sense to me, but I think the default should be latest and greatest unless upstream is saying "yes, we are definitely going to delete this stuff" ... upstream has a good reason to keep it, since they have their own huge JS app that doesn't have to be cross-browser.
(In reply to comment #9) > Where is this bug coming from? Did upstream indicate somehow that this was > likely? Nope, just my intuition. > Adding ability to downgrade makes sense to me, but I think the default should > be latest and greatest unless upstream is saying "yes, we are definitely going > to delete this stuff" ... upstream has a good reason to keep it, since they > have their own huge JS app that doesn't have to be cross-browser. Yeah, except they have shown multiple times a willingness to change their entire code base if it makes sense for them. I don't have control over the code bases of everyone using Gjs necessarily, and I want the default to be conservative, not whatever stuff Brendan thought would be cool last year.
And I know this was debated at length before, but I'm definitely coming down on the side of at least allowing JS consumers the possibility of switching engines. Now, SpiderMonkey has some custom stuff exposed even in the Web JS, like: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/proto Which is obviously huge for gnome-shell. But it's marked "Non-Standard", and tellingly, "Deprecated". Will they remove that? Hard to know...but note that Mozilla's own MDN describes how authors can avoid a dependency on it: https://developer.mozilla.org/en/JavaScript/Guide/Inheritance_Revisited
I dunno, to me "let" and "yield" are both worth the risk because they are well-considered and sorely needed extensions to the language. If others want to take near-term pain and do near-term work to avoid hypothetical future pain and work, then OK, but for me I'd rather avoid the definite immediate headache ;-) Maybe we should get some other gjs users to weigh in. I guess the defaults should depend on what most people want to do. Could also just discuss this w/ spidermonkey guys a bit and see if they've considered what to do with the extensions.
(In reply to comment #12) > I dunno, to me "let" and "yield" are both worth the risk because they are > well-considered and sorely needed extensions to the language. Those two I see as in quite different classes; 95% of code changes it's trivial not to use "let". For the inner loops, yes you have to do some code rewriting potentially to lift assignments, but it's not *hard*. There's obviously no trivial way to not use "yield". But I do think that should be something you have to opt-in to, since it e.g. precludes your code from running on V8 in a very nontrivial way. > Maybe we should get some other gjs users to weigh in. I guess the defaults > should depend on what most people want to do. Note this patch set isn't changing the default - yet. It just allows consumers to choose the version. But obviously, I want to do that in the future. Again at a high level I think it makes sense to view this patch set as making us behave similarly to Firefox. There, you have to opt in to the extensions too.
See also from today, actually: http://adblockplus.org/blog/rewriting-javascript-code-with-jshydra
So, can we go ahead with these patches now to allow specifying the version, so I can change e.g. gnome-shell to opt into JS 1.8 now, without having to do it all at once?
(In reply to comment #8) > Review of attachment 175967 [details] [review]: > > ::: gjs/context.c > @@ +737,3 @@ > + */ > +const char * > +gjs_context_scan_buffer_for_js_version (const char *buffer, > > it's tough for me to see how the app would know what to use for maxbytes (other > than, the full size of the file, which sucks). maybe "on the first or second > line" should be the default / only option ? I'd say "on the first line" but I > guess emacs magic or #! might be there. Well, I don't expect "apps" to use this; pure scripts are going to be going through whatever gjs/console.c does. Embedders are much more likely to specify js-version as part of g_object_new(GJS_TYPE_CONTEXT, "js-version", "1.8", NULL) etc. But the reason I added a length parameter is for the case where you e.g. mmap a file; there you have to care about the length, otherwise we could just run off the end of the memory map.
Yes, allowing specifying the version makes sense, go for it. I am just wondering if we should really set the defaults to least-common-denominator. Uint8Array is another extension you just brought up that's awfully nice to have... Agreed a length is needed to avoid reading invalid memory, what I'm worried about is the inefficiency of scanning every file to the end, which would happen if a typical thing is to not specify a version... so maybe putting the version in an arbitrary part of the file shouldn't be allowed, meaning the scan could end on the second newline or whatever instead of the end of the file. The buffer length arg's semantics would be "give up at this length, OR on second newline" rather than "scan up to this length" Or maybe think about ways to get the JS version "out of band," just quickly thinking for example, modules could be installed in a versioned directory, and scripts could pass the version in to gjs-console as part of the #! ... (do we need versioned modules anyhow or will an error just naturally happen if you try to use a module that requires a newer JS?)
I just want to note here that I did verify that doing: -#define _GJS_JS_VERSION_DEFAULT "1.8" +#define _GJS_JS_VERSION_DEFAULT "0" the tests still pass. So this double checks that specifying a version in the files does work.
Attachment 175964 [details] pushed as 178c4d9 - context: Use G_TYPE_STRV for search-path Attachment 175965 [details] pushed as 21ae630 - Makefile-test: Fix RUN_WITH_DBUS usage Attachment 175966 [details] pushed as b11c8d3 - context: Add a "js-version" GObject property Attachment 175967 [details] pushed as 11a3183 - context: Add functions to scan a file/buffer for a JS version Attachment 175968 [details] pushed as f4cb2b7 - console: Detect input JS version, add --js-version argument Attachment 175969 [details] pushed as 70ba6df - gjs-unit: Pull in JS version from header in tests; add version to tests Attachment 175970 [details] pushed as c257bbe - Add explicit tests of JS version functionality
Leaving open this bug for comments about changing the default version to "default". I suggest, regardless of the outcome, applications and embedders *now* change to explicitly specify "1.8" if you need that.
The JSVersion stuff is still present in SpiderMonkey, but I'm not sure it does anything anymore, as they stopped at 1.8.5. Closing this as I don't think there's anything more to do here.