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

BREAKING CHANGE: Use ES2015 syntax #84

Merged
merged 1 commit into from
Oct 16, 2017
Merged

BREAKING CHANGE: Use ES2015 syntax #84

merged 1 commit into from
Oct 16, 2017

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented Oct 16, 2017

This is related to #82 but not really breaking compat with Node.js 4.

This PR converts:

  • var func = function() { } style to function func() { }
  • var x to let x or const x depending on the variable is reassigned or not
  • ES5-style class declaration for WebIDLParseError to ES2015 one
  • for (var i = 0; i < array.length; i++) to for (const item of array)
  • callback functions to use arrow syntax
  • object literal member functions { func: function () { } } to { func() { } }

PS: Now fixes #82 by removing it on .travis.yml.

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits.

lib/webidl2.js Outdated
JSON.stringify(this.tokens, null, 4);
};
toString() {
return this.message + ", line " + this.line + " (tokens: '" + this.input + "')\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use template literal here?

lib/webidl2.js Outdated
function error(str) {
let tok = "";
let numTokens = 0;
const maxTokens = 5;
while (numTokens < maxTokens && tokens.length > numTokens) {
tok += tokens[numTokens].value;
numTokens++;
}
throw new WebIDLParseError(str, line, tok, tokens.slice(0, 5));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should fix this now:

throw new WebIDLParseError(str, line, tok, tokens.slice(0, maxTokens))

@@ -714,15 +718,15 @@
consume(OTHER, ";") || error("Missing semicolon after interface");
return ret;
}
var ea = extended_attrs(store ? mems : null);
const ea = extended_attrs(store ? mems : null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: whitespace ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be nice if we got rid of the const x = {}, y = {}; etc. pattern usage and instead do "const x = {}; const y = {}" and so on. The "," is a pretty bad practice. We can do that in different PR tho.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: whitespace ?

Could you clarify more? extended_attrs already calls all_ws internally so no need to process whitespaces twice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I meant changing:

const mems = [],
  ret = {};

To:

const mems = [];
const ret = {};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to whitespace.... I literally meant that the indentation of your code is incorrect - just a nit :) The actual processing is all good 👍

@marcoscaceres
Copy link
Member

We need to update the travis file as per #82 (i.e., drop node 4):

node_js:
  - "lts/*"
  - "6"

I don't know if this works, but we could try:

node_js:
  - "lts/*"
  - "lts/6"

@saschanaz
Copy link
Member Author

We need to update the travis file as per #82 (i.e., drop node 4):

Not adding node will exclude non-LTS latest stable versions (9, 11, ...), do we want to exclude them?

@marcoscaceres
Copy link
Member

Not adding node will exclude non-LTS latest stable versions (9, 11, ...), do we want to exclude them?

Ah, good point. Yeah, we probably want to test whatever is latest. I have to run off for a bit, can you please check travisci docs for how to do that?

@saschanaz
Copy link
Member Author

saschanaz commented Oct 16, 2017

https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#Specifying-Node.js-versions

node latest stable Node.js release

Current node points to the latest version. Not sure whether lts/6 will work or not but it's probably not good to use an undocumented string.

@saschanaz
Copy link
Member Author

saschanaz commented Oct 16, 2017

lts/6 didn't work but the documentation says lts/boron works.

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saschanaz, sent you an invite with write access. Please merge.

I'll release after merge.

@marcoscaceres marcoscaceres changed the title Use ES2015 syntax BREAKING CHANGE: Use ES2015 syntax Oct 16, 2017
@marcoscaceres
Copy link
Member

Please squash merge and use "BREAKING CHANGE: Use ES2015 syntax" as commit message.

@saschanaz saschanaz merged commit fa4b9a3 into w3c:develop Oct 16, 2017
@saschanaz saschanaz deleted the no-var branch October 16, 2017 11:14
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.

None yet

2 participants