Chromium JavaScript Style guide: 'const'

For ES6/Harmony features like const, the style guide is superseded by the ES6 Support document, which lists const as 'To Be Discussed'.

Some extra concerns besides performance are whether it is supported by uglifyjs (it seems like it is) and by iOS (seemingly iOS 10+ only).

On 22 March 2017 at 12:16, Eric Lawrence <[email protected]> wrote:

The Chromium JavaScript style guide no longer mentions that 'const' is not permitted in JavaScript. 

That prohibition was dropped in October 2016 in Revision #50. The V8 Issue cited as the original justification was marked as "Won't Fix" with the
note that the behavior of 'const' had changed and should not have the performance impact that led to its original ban.

In a recent CL, I was told that the DevTools still do not use const and existing usage is being removed due to V8 deoptimization concerns.

Should the ban (or a warning) be added back to the JavaScript style guide? Will this concern go away in a future version of V8?

On Wed, Mar 22, 2017 at 2:26 PM, Tim Sergeant <[email protected]> wrote:

For ES6/Harmony features like const, the style guide is superseded by the ES6 Support document, which lists const as 'To Be Discussed'.

Some extra concerns besides performance are whether it is supported by uglifyjs (it seems like it is) and by iOS (seemingly iOS 10+ only).

Yeah, I suspect tooling and feature support are likely more the issue these days.

 

On 22 March 2017 at 12:16, Eric Lawrence <[email protected]> wrote:

The Chromium JavaScript style guide no longer mentions that 'const' is not permitted in JavaScript. 

That prohibition was dropped in October 2016 in Revision #50. The V8 Issue cited as the original justification was marked as "Won't Fix" with the
note that the behavior of 'const' had changed and should not have the performance impact that led to its original ban.

In a recent CL, I was told that the DevTools still do not use const and existing usage is being removed due to V8 deoptimization concerns.

Should the ban (or a warning) be added back to the JavaScript style guide? Will this concern go away in a future version of V8?

It likely depends on where and how you're using it.  It's probably fine but you should talk with somebody that works on v8 / turbofan to be sure.  +adamk@ 

Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

On Wednesday, March 22, 2017 at 8:49:47 AM UTC-7, Adam Klein wrote:

On Tue, Mar 21, 2017 at 10:30 PM, Dan Beam <[email protected]> wrote:

On Wed, Mar 22, 2017 at 2:26 PM, Tim Sergeant <[email protected]> wrote:

For ES6/Harmony features like const, the style guide is superseded by the ES6 Support document, which lists const as 'To Be Discussed'.

Some extra concerns besides performance are whether it is supported by uglifyjs (it seems like it is) and by iOS (seemingly iOS 10+ only).

Yeah, I suspect tooling and feature support are likely more the issue these days.

 

On 22 March 2017 at 12:16, Eric Lawrence <[email protected]> wrote:

The Chromium JavaScript style guide no longer mentions that 'const' is not permitted in JavaScript. 

That prohibition was dropped in October 2016 in Revision #50. The V8 Issue cited as the original justification was marked as "Won't Fix" with the
note that the behavior of 'const' had changed and should not have the performance impact that led to its original ban.

In a recent CL, I was told that the DevTools still do not use const and existing usage is being removed due to V8 deoptimization concerns.

Should the ban (or a warning) be added back to the JavaScript style guide? Will this concern go away in a future version of V8?

It likely depends on where and how you're using it.  It's probably fine but you should talk with somebody that works on v8 / turbofan to be sure.  +adamk@ 

If worried about deopt issues, the best strategy would be to wait until we're fully on the ignition/turbofan pipeline, which is targeting M59. The deopt sometimes seen with const (and let) is due to Crankshaft, which will be retired once the new pipeline is the default.

 

- Adam

--
Chromium Developers mailing list: [email protected]
View archives, change email options, or unsubscribe:
https://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

On Tue, Jun 13, 2017 at 4:16 PM, 'John Williams' via Chromium-dev <[email protected]> wrote:

Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

I don't see anything about const in the markdown version of the web styleguide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md

However, it is an ES6 feature, and there's a separate doc about what we've vetted so far:

https://chromium.googlesource.com/chromium/src/+/master/docs/es6_chromium.md

It'd make sense to cross-link or combine these docs eventually.

Many Chromium .js files are run through tools that parse source code.  Examples: closure compiler, vulcanize/polymer-bundler, eslint, uglify.  It's generally preferred that they all handle new syntaxes well before allowing everywhere (because it's hard to express/follow "this feature is allowed but only for some directories").  I'm also leary of potential performance problems, so we've been consulting the v8 team where possible (which is why I added adamk@ on your previous email).

Uglify was the most recent tool holding us back and was very recently updated to better handle ES6 features.  Doing compatibility research is the fastest and safest way to introduce your feature.

tl;dr - if we want to use const, I think it should be proposed to chromium-dev@ like I did with => (which we can probably finally start using now).  This matches what we did for C++, as described here.  Might make sense to propose let and const together.

(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

That simplifies things, but I believe Chrome for iOS still supports iOS 9 which has dodgy ES6 support.  If you use ES6 features (like const) in src/ui/webui/ it could break stuff.  https://kangax.github.io/compat-table/es6/ says that iOS9 supports only a small part of const.

Looking at the style guide, I see quite a few ES6 features we use that are in the "to be discussed" category. I'm worried this means I'll need to either get a whole lot of ES6 features approved, or make a lot of changes to the code I'm releasing that will be generally detrimental to the quality of the code. That seems like a high barrier to meet just to include some code in the Chromium tree that is already in production as a de-facto part of Chrome.

On Wed, Jun 14, 2017 at 10:31 AM, Dan Beam <[email protected]> wrote:

On Tue, Jun 13, 2017 at 4:16 PM, 'John Williams' via Chromium-dev <[email protected]> wrote:

Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

I don't see anything about const in the markdown version of the web styleguide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md

However, it is an ES6 feature, and there's a separate doc about what we've vetted so far:

https://chromium.googlesource.com/chromium/src/+/master/docs/es6_chromium.md

It'd make sense to cross-link or combine these docs eventually.

Many Chromium .js files are run through tools that parse source code.  Examples: closure compiler, vulcanize/polymer-bundler, eslint, uglify.  It's generally preferred that they all handle new syntaxes well before allowing everywhere (because it's hard to express/follow "this feature is allowed but only for some directories").  I'm also leary of potential performance problems, so we've been consulting the v8 team where possible (which is why I added adamk@ on your previous email).

Uglify was the most recent tool holding us back and was very recently updated to better handle ES6 features.  Doing compatibility research is the fastest and safest way to introduce your feature.

tl;dr - if we want to use const, I think it should be proposed to chromium-dev@ like I did with => (which we can probably finally start using now).  This matches what we did for C++, as described here.  Might make sense to propose let and const together.

I'll do that.

 

(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

That simplifies things, but I believe Chrome for iOS still supports iOS 9 which has dodgy ES6 support.  If you use ES6 features (like const) in src/ui/webui/ it could break stuff.  https://kangax.github.io/compat-table/es6/ says that iOS9 supports only a small part of const.

I think maybe I'm missing something. Are you saying the Chromium tree contains JS code that needs to run in Safari on iOS 9, and that's a sufficient reason to ban const in the entire Chromium tree?

On Tue, Jun 27, 2017 at 10:16 PM, 'John Williams' via Chromium-dev <[email protected]> wrote:

On Wed, Jun 14, 2017 at 10:31 AM, Dan Beam <[email protected]> wrote:

On Tue, Jun 13, 2017 at 4:16 PM, 'John Williams' via Chromium-dev <[email protected]> wrote:

Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

I don't see anything about const in the markdown version of the web styleguide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md

However, it is an ES6 feature, and there's a separate doc about what we've vetted so far:

https://chromium.googlesource.com/chromium/src/+/master/docs/es6_chromium.md

It'd make sense to cross-link or combine these docs eventually.

Many Chromium .js files are run through tools that parse source code.  Examples: closure compiler, vulcanize/polymer-bundler, eslint, uglify.  It's generally preferred that they all handle new syntaxes well before allowing everywhere (because it's hard to express/follow "this feature is allowed but only for some directories").  I'm also leary of potential performance problems, so we've been consulting the v8 team where possible (which is why I added adamk@ on your previous email).

Uglify was the most recent tool holding us back and was very recently updated to better handle ES6 features.  Doing compatibility research is the fastest and safest way to introduce your feature.

tl;dr - if we want to use const, I think it should be proposed to chromium-dev@ like I did with => (which we can probably finally start using now).  This matches what we did for C++, as described here.  Might make sense to propose let and const together.

I'll do that.

 

(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

That simplifies things, but I believe Chrome for iOS still supports iOS 9 which has dodgy ES6 support.  If you use ES6 features (like const) in src/ui/webui/ it could break stuff.  https://kangax.github.io/compat-table/es6/ says that iOS9 supports only a small part of const.

I think maybe I'm missing something. Are you saying the Chromium tree contains JS code that needs to run in Safari on iOS 9, and that's a sufficient reason to ban const in the entire Chromium tree?

Some of the javascript code is run by Chrome on iOS. This includes some code for webui, but also some code from components (like translate code, or distiller code). This javascript will be run in a WKWebView that uses the same JS engine as Safari on iOS. That shared code still need to run on iOS and the version of the OS supported is iOS 9 and higher.

So if Safari does not support "const", then we should ensure that all javascript run on Chrome on iOS (from the Chromium repository) does not use features unsupported. I don't think we should ban "const" just for that reason if we can find a way to make the code work on iOS (maybe we can transpile from ES6 to ES5 or something else).

-- Sylvain

One of the reasons we did not decide to blindly allow all ES6 features is because various JS tools (for example Closure Complier and Uglify) used to not support them well (either crashing or producing erroneous output). The effort to ensure that all our tools support ES6 is tracked by issue 671426. All currently whitelisted ES6 features have been tested with those tools. Also IOS adds a layer of complication, since features that are supported in Chrome's ToT, are not necessarily support in all IOS versions.

Part of the process of proposing a new feature (like let/const) is verifying that nothing in the toolchain crashes as a result of that. So, since I own that bug, I'll take this as an action item to verify let/const are well supported and put up a CL to update the styleguide.

Now regardless of the JS toolchain ES6 support, the other reason we've setup a process of proposing ES6 features, one by one, is because not necessarily all ES6 (or ES7) features will be non-controversial to use (similar to the C++11 feature proposal process). The ones that come to mind that fall in this category are generators, and async/await.

On Wed, Jun 28, 2017 at 12:14 AM, Demetrios Papadopoulos <[email protected]> wrote:

One of the reasons we did not decide to blindly allow all ES6 features is because various JS tools (for example Closure Complier and Uglify) used to not support them well (either crashing or producing erroneous output). The effort to ensure that all our tools support ES6 is tracked by issue 671426. All currently whitelisted ES6 features have been tested with those tools. Also IOS adds a layer of complication, since features that are supported in Chrome's ToT, are not necessarily support in all IOS versions.

My team's codebase has been developed from the start with the Closure compiler, so I know we're not using any features it doesn't support well. It's a Chrome extension, so there's no scenario in which it could execute outside of Chrome proper. Is there any why to distinguish code in the repo that needs to run in Safari vs. code that doesn't?

 

Part of the process of proposing a new feature (like let/const) is verifying that nothing in the toolchain crashes as a result of that. So, since I own that bug, I'll take this as an action item to verify let/const are well supported and put up a CL to update the styleguide.

How would I go about testing that? So far we know for sure it works the the Closure compiler and linter. Are there other tools that need to be tested?