-
Notifications
You must be signed in to change notification settings - Fork 1.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
[ES6] Make distinction between * method and * operator #1229
Conversation
This supersedes #1227, right? |
Yes |
Run with PR #1229:
Edit: furthermore:
|
Errors too soon |
Better to add an identifier check |
I made that example because the PR was special casing |
Seems to be a recent breakage |
Actually we shouldn't need this very special case at all (too many cases that ends up being invalid identifiers) |
When you fix it, could you add test cases for For bonus points, test setters/getters using some unprintable unicode names like "\x00\x01" with |
This issue needs lots of test cases anyway :-) |
Basically, all the logic we need was there in ObjectKeyVal |
@avdg @rvanvelzen Here's an idea for Edit: the re-parsed output need not necessarily match the original expected test output, which is fine - just as long as it it doesn't error out. |
return "bar"; | ||
}, | ||
*"%"() { | ||
return "foobar"; |
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.
You don't need any return statements here at all for the purpose of this test to make the expect_exact
string shorter. Or you can just have 1; 2; 3; etc, if you want to easily visually cross reference the specific failing part of the test.
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'll replace them with numbers
Well, we shouldn't care about ie8 with method naming, but we still have getters that have modified behaviour and don't support ie8. |
Failing test is due to timeout on node 0.12 |
Please increase the timeout of this specific mocha test to 15000 to compensate for occasionally slow test machines. |
Just add the increased timeout for this unrelated test failure to this PR - don't bother making a new PR. |
Done, any more test cases? |
PR #1230 exposed 4 test failures on the harmony branch. Please test with it. |
PR #1230 would have caught the |
2 errors related to surrogated identifiers |
Yeah, they are unrelated issues to this PR. But test was inspired by error in this PR. |
|
*"\x00\x01"() { | ||
return "foobar"; | ||
} | ||
} | ||
} | ||
expect_exact: 'var foo={"\\0\x01":"foo",get"\\0\x01"(){return"bar"},*"\\0\x01"(){return"foobar"}};class bar{get"\\0\x01"(){return"bar"}*"\\0\x01"(){return"foobar"}}' | ||
expect_exact: 'var foo={"\\0\x01":"foo",get"\\0\x01"(){return"bar"},set"\\0\x01"(foo){save(foo)},*"\\0\x01"(){return"foobar"}};class bar{get"\\0\x01"(){return"bar"}set"\\0\x01"(foo){save(foo)}*"\\0\x01"(){return"foobar"}}' | ||
} |
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.
Can you make a new test that's a copy of the test property_with_unprintable
except using ascii_only=true
?
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.
Done
Uh, we do have to store these quotes no? (keep quotes option) |
Probably should store the quote but not emit them by default if not required. It should respect |
Normal properties seems fine, just the getters and setters not |
Current progress: property shortcut not quoted properly yet as I do have to find out a printing strategy yet when I have more time. |
@kzc I better squash the commits for now since the fix for the star alone seems to be important. There are other broken stuff, but they aren't regressions just yet. |
Also add quotes to properties when necessary, this might be the case if the name isn't a valid identifier
done |
I've added more tests on the generator-symbol-tmp branch. Edit: since the merge, this should now be avdg/UglifyJS2@harmony...avdg:generator-symbol-tmp |
(note on additional optional patches) I might move the concise method thing to an AST_ObjectProperty, given the getters are functions as well and doing fine there. Edit; AST move should be done there, I still have issues with computed properties as concise method, which seems to make the scope analyzer complain a lot. Still investigating cause, but it has something to do with things like Edit2: Seems that is solved by adapting the walker, maybe this can be generalized in such a way we don't need AST_ObjectComputedKeyVal anymore. |
You may just want get this one merged to address the regression and wait to add features in other PRs. |
Yes yes, that's what I'm doing right now. I want to prevent any more unnecessary blockages. Although I can go a step further by opening a separate issue/pr. |
Just a note, jspm often generates output like: System.registerDynamic("npm:core-js@2.4.1.json", [], false, function() {
return {
"main": "index.js",
"format": "cjs",
"meta": {
"*": {
"globals": {
"process": "process"
}
},
"*.json": {
"format": "json"
}
}
};
}); This PR prevents the harmony branch from crashing at the "*" key. |
Awesome, thanks! |
No description provided.