New syntax and extra #22100
New syntax and extra #22100
Conversation
|
Nice work 💯 It is great to see updated code. I love pull requests where the number of lines deleted are greater than the number added. 👍 FYI the tests might continue to fail until #22050 is merged. So don't bang your head if they are failing and you can't figure out why. lol |
There was a problem hiding this comment.
There are some issues with let/const usage. You have used let in many places where the variable is used only once.
For these kinds of changes, you should use automatic tools that verify the changes using JavaScript AST which is more accurate than human eyes.
The whole var conversion can be done using jscodeshift.
npm install -g jscodeshift
git clone https://github.com/cpojer/js-codemod.git
jscodeshift -t js-codemod/transforms/no-vars.js ./src
| @@ -611,11 +611,11 @@ module.exports = class Cursor extends Model { | |||
| : new Range(position, new Point(position.row, Infinity)); | |||
|
|
|||
| const ranges = this.editor.buffer.findAllInRangeSync( | |||
| options.wordRegex || this.wordRegExp(options), | |||
| options.wordRegex ?? this.wordRegExp(options), | |||
There was a problem hiding this comment.
Neither Node 12 nor Babel 5 support nullish coalescing
These changes require merging #21623
| @@ -63,7 +63,7 @@ module.exports = class DeserializerManager { | |||
| const deserializer = this.get(state); | |||
| if (deserializer) { | |||
| let stateVersion = | |||
| (typeof state.get === 'function' && state.get('version')) || | |||
| state.get?.('version') ?? | |||
There was a problem hiding this comment.
Neither Node 12 nor Babel 5 support optional chaining or nullish coalescing
These changes require merging #21623
I'm surprised this doesn't work
Update. Pun
Co-authored-by: Tony Brix <tony@brix.ninja>
Co-authored-by: Tony Brix <tony@brix.ninja>
I don't have access to a terminal right now so aminya's suggestions will happen later
There was a problem hiding this comment.
@icecream17 This PR is already very hard to verify. Could you make separate PRs for editing individual changes (e.g. one PR for no-var conversion). Also, the commit messages are written very generically. Please be specific/verbose to help the reviewers.
|
Closing (will make seperate prs in the future) |
This pr uses new javascript syntax
Also by using the new javascript syntax it could change functionality a bit
Also Deepcode noticed some things: https://www.deepcode.ai/app/gh/atom/atom/2beb93b0d7a0447150f52a9326d2c925e64254d2/_/dashboard and I've fixed some problems.
Quantitative Performance Benefits
Well, the build doesn't even succeed yet (#22099). But I think updating will be better for performance
Possible Drawbacks
Updating
Verification Process
The build command doesn't work, but
atom-beta --testdoes. And no errors, so that's great.Note on commit 1
In the quicksort functions, or some other sort function, I think I changed "shift" to "pop". It's the same thing but faster.I changed the (Number) thing in some file to warn that binary, octal, and hex literals aren't supported now. (Because the old code doesn't check for this - it uses parseFloat.) Number is generally better than parseFloat but not that much. This doesn't have any affect on the code though.
I randomly've gone through a whole bunch of files so this commit is messy
Sometimes I wasn't sure if `??` could be used, and on those times, a few times I put it there anyways because any code that depends on the obscure exceptions should be fixed. But mostly I didn't change when I was unsure due to laziness
CONTRIBUTING.MD changed, but only for consistency. This was caught by some extension in vscode.
Note on recent commits
Most of them just reference that Uzitech originally suggested these changesSome of them are lint fixes
And then there's one commit that fixed a syntax error
TODO:
Edit: Depends on #21623
View rendered CONTRIBUTING.md