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

replaced javascript keywords with non-conflicting alternatives #13065

Closed
wants to merge 3 commits into from

Conversation

dr0ps
Copy link

@dr0ps dr0ps commented Jan 8, 2018

I use the YUI Compressor to generate a compressed version of three.js and the additional files used in my project. YUI Compressor requires a very strictly valid javascript syntax and fails because javascript keywords are used as parameter or variable names in several places of three.js. After my changes the YUI Compressor does no longer complain.
I think I applied all suggested changes from the last try but I also rebased the commit on a current dev branch.

@looeee
Copy link
Collaborator

looeee commented Jan 8, 2018

boolean is not a JavaScript keyword. Boolean is, but the capitalization is important.

It's YUI compressor that is incorrect here, not three.js.

It's also a dead project as far as I can see - barely any updates in the last couple of years. You should probably switch to a different minifier.

@looeee
Copy link
Collaborator

looeee commented Jan 8, 2018

Also, this is exactly the same as your previous PR here: #13065
You were asked to use bool as an replacement for boolean the last time.

If you make that change this will probably be accepted, since bool tends to be the standard parameter name anyway.

@dr0ps
Copy link
Author

dr0ps commented Jan 8, 2018

So I changed it all to "bool".

@mrdoob
Copy link
Owner

mrdoob commented Jan 11, 2018

boolean is not a JavaScript keyword. Boolean is.

👍

@mrdoob mrdoob closed this Jan 11, 2018
@dr0ps
Copy link
Author

dr0ps commented Jan 11, 2018

This is ridiculous. Lowercase boolean is a keyword in all Javascript/Ecmascript standards except ES5/6. Uppercase Boolean is not a keyword at all but the class name for the wrapper object. It is probably very much ok to have your own Boolean. It is not ok to use boolean except for ES5/6.

@mrdoob
Copy link
Owner

mrdoob commented Jan 11, 2018

@dr0ps do you have any links that support your claim?

@looeee
Copy link
Collaborator

looeee commented Jan 11, 2018

Actually @dr0ps is partially correct, boolean was a "Future reserved keyword" in ES1-3.

That is, it was never actually a keyword but was reserved as a potential keyword.

See "Future reserved keywords in older standards" on this mdn page.

We could merge this as it's a relatively minor change, but I think that the onus is on users like @dr0ps to ensure that they are using libraries that are compliant to the current spec. Or at least a fairly recent spec. ES3 has been outdated since ES5 was released in 2009.

@dr0ps
Copy link
Author

dr0ps commented Jan 12, 2018

Sure. The ECMAscript 6 Lexical Grammar says "The following are reserved as future keywords by older ECMAScript specifications (ECMAScript 1 till 3)" and it lists both "boolean" and "char".
https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Lexical_grammar
And the W3C explicitly states "Do not use these words as variables. ECMAScript 5/6 does not have full support in all browsers".
https://www.w3schools.com/js/js_reserved.asp

@looeee
Copy link
Collaborator

looeee commented Jan 16, 2018

@dr0ps w3schools is a good place for learning JS but should not be taken as a definitive source for spec info. You should refer to mdn instead.

Unless you can find a source that says otherwise, I think we can assume that all modern browsers are fully ES5 compliant (or at least very close to), and we don't need to follow older spec versions anymore.

Again, I would suggest that you change to a different library for minification, since the one you are using is outdated.

@mrdoob mrdoob added this to the r90 milestone Jan 17, 2018
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.

3 participants