js operators

[icecream17]

• Previously blocked by Add more operators tree-sitter/tree-sitter-javascript#152 (and other updating prs)
• Previously blocked by ⬆️ update tree-sitter packages to 0.19.0 atom/language-javascript#692
  • Blocked until Uncaught RangeError after updating tree-sitter atom/atom#22129 is fixed tree-sitter is only partially fixed, but legacy textmate is done

Description of the Change

Adds optional chaining a?.b a?.[5] a?.()
Adds exponentiation, **= **
Adds logical assignment &&= ||= ??= and nullish operator ??
Add shift specifier (css class) to >>= >>>= <<= (non-tree-sitter)
Add void (it's not new but it's missing) (tree-sitter)

I've added tests for the ?. operator, and included ?? in the logic operator test. (Works!!!!!)
One test is deleted because it's a duplicate

Port of the original pr

Alternate Designs

I didn't try too much on tree-sitter scopes because of the possible atom/language-javascript#690

Benefits

Fixes atom/language-javascript#640
Fixes atom/language-javascript#680
Fixes atom/language-javascript#715 (duplicate)

Possible Drawbacks

Some comments

The non-tree sitter version is almost completely copied so nothing to say there.

There are some inconsistencies/missing operators in the tree-sitter version, for example:

• this
• new.target
• import.meta
• the keyword.operator scoped operators are ordered more incorrectly the more I look at it, but again not efforting because of Implement naming conventions in JavaScript and Regex grammars atom/language-javascript#690

Applicable Issues

atom/language-javascript#640
atom/language-javascript#680
atom/language-javascript#715

Todo

• Update JS tests
• Remove rest scope since there's no way to tell whether it's rest or spread (Edit: In some contexts you can guarantee one or the other)

Unrelated

• use let/const es6 syntax for, forin, forof atom/language-javascript#691 lgtm, seems like a (good first issue) edit: use let and const in js snippets #326

[kaosine]

Is there any way to also put this on pulsar-edit/core-languages? I want to use this as a test for importing things that way, so that hopefully it's easier to update those without having so much in here....especially, so maybe it's easier to disable stuff that doesn't work or whatever as needed....

[icecream17]

how does the core-languages repo work?

[kaosine]

It's basically going to be an organized mono-repo of our language support as a test to organizing everything in regard to a discussion in the org level stuff. Just haven't been able to get it all uploaded, since just migrating over this specific language's commit history is taking forever on my end if I did it right in fish.

Since I'm trying to migrate(have it synced) it with this command as to what the current status is of that directory over here:

git log --pretty=email --patch-with-stat --reverse --full-index --binary -m --first-parent -- packages/languages-javascript >> (cd ../packages/core-languages && git am --committer-date-is-author-date)

I probably need to do even more research on how to do this right since I just kinda went with the first SO that I found that seemed to be what I needed and just converted the command to fish terms...

[icecream17]

what's the status of core-languages? / this pr?

[Spiker985]

@icecream17 We've actually monorepo'd the languages into the pulsar repo, so this is still the proper location for it.

If this request is ready for review, we can move it from a Draft status into a Ready status

[icecream17]

the language-js tests still need to be updated since some of them are failing, working on it

[icecream17]

don't understand why the >>= et al shift operators are failing especially since those tests passed at atom/language-js ... even weirder is that the right side of the to be message is wrong

[icecream17]

Woah #299 might've been legendary, can someone retry the tests?

Edit: Also marking as ready since I don't plan on making changes unless there's some bug

[confused-Techie]

Woah #299 might've been legendary, can someone retry the tests?

Not sure we need to retry them, when I authored that PR it fixed all tests for that package locally, and iirc resolved them in the pr tests

[confused-Techie]

Woah #299 might've been legendary, can someone retry the tests?

Edit: Also marking as ready since I don't plan on making changes unless there's some bug

Would you mind updating this branch from master? Since especially here our tests have improved a ton

[icecream17]

I clicked a button to merge 504 commits, hopefully that "just works"

[confused-Techie]

So that's for merging the master branch here, but it seems on the language test package bundle, we are having 34 failures when we would expect only 18 on this bundle.

So something isn't quite happy. If you'd like a reference for the 18 tests that we expect to fail compared to yours, you can look at the tests from this recent PR

[kaosine]

@icecream17 We've actually monorepo'd the languages into the pulsar repo, so this is still the proper location for it.

If this request is ready for review, we can move it from a Draft status into a Ready status

Yeah it was me trying something and just wanting a duplicate for that side of things even tho it didn't work out the way I wanted too(and I think languages may have changed and broken anyways since that point from what I was trying to do....)

[confused-Techie]

Alright, so like I mentioned we are getting some more failures here than we would hope.

But I would like to see us be able to merge these changes if they improve our handling of JavaScript, so maybe once #307 is merged I'll go ahead and update this PR from master and see if we are still getting the failures. (That PR contains many many fixes to our test runner, tests, and easier viewing of the test output after the fact. Meaning we can say exactly this causes these tests in language-javascript to fail, or know they are unrelated.

Thanks for the patience

[icecream17]

I've gone ahead and merged 162 commits to include the now merged #307

[icecream17]

And here's some affected code

non-tree-sitter

let a;
a ??= a ?? true
a **= a ** true
a &&= false
a ||= true
a |= 5
a &= 4
a ^= 3
a <<= 2
a >>= 1
a >>>= 0
(...rest) => [...spread]

obj?.prop
a()?.b()
a.b?.()
a()?.MY_CONSTANT
a?.b()
a?.b?.()
a?.[3]
a[4]?.()
new a?.b // < should display as illegal

// useless ?. support in api
Math?.PI
Relay?.QL``

// barely any difference (now detected as `support.function.json` or similar instead of `entity.name.function`)
// similar to the difference between `console.log` and `console.customfunction`
// some api additions, like
JSON?.parse()
console.timeStamp()
Promise.allSettled()
BigInt
WeakRef

tree-sitter

let a;
a ??= true
a &&= false
a ||= true
a **= a ** a
a = [2, 3]
delete a[1]
a = void a 

[confused-Techie]

@icecream17 I really appreciate you getting this branch updated with all of our new CI. Made things super easy to review.

Firstly the intel_mac build failed, but seems to be some of the flakiness within node-gyp and not the cause of your changes.

As for package tests we have the following which looks great:

• find-and-replace: 50 Failures (Within Expected Range)
• symbols-view: 2 Failures (Expected 2)
• tree-view: 2 Failures (Expected 2)

But then unfortunately we do have some failures within language-javascript which would be awesome if you could take a look at:

• language-javascript: 2 Failures - 0 Expected

JavaScript grammar ES6 tagged Relay.QL string templates tokenizes them as strings
JavaScript grammar ES6 tagged Relay.QL string templates with interpolation tokenizes them as strings

[confused-Techie]

So I've just now seen that @icecream17 had fixed the tests when I originally requested.

Just wanna say sorry this has been around for so long without further discussion.

But at this point the changes here look solid, and tests seem to be happy with it.

So thanks again for all the awesome work here, we really appreciate it!

As for everyone else @pulsar-edit/core how do we feel about merging this one?

And for some confirmation, the tests:

• find-and-replace: 58 Failures, within expected range
• symbols-view: 2 Failures, within expected range
• tree-view: 2 Failures, within expected range
• intel_mac build: Seems to have failed when building native modules. I think this was an issue we had fixed after bumping NPM in PPM?
• windows: Timed out, so likely not an actual issue with this PR

So with that said, things look basically where they should be, but even for those two failures I don't expect them to be the fault of this PR. So how do we feel about merging? And getting some modern features supported by language-javscript?

[DeeDeeG]

I will re-run the Cirrus CI.

[DeeDeeG]

Oh, the intel macOS Cirrus thing is down to #384 not being on this "older" branch.

October 2, 2022

👀 ❗

I'll run a branch with that fix just to make sure it passes CI, no need to complicate the commit history on this PR branch I'll do it separately.

EDIT: See status of the CI runs here: icecream17-js-operators-rebase --> https://cirrus-ci.com/build/4880197007179776

EDIT 2: Cirrus is looking good now. (See links just above).

[DeeDeeG]

✅ lite approve, as language grammars are a bit over my head, but I like the spirit of this, and it's something I can generally be positive on.

I leave more in-depth and technical reviewing to others who may be more familiar with this stuff.

[icecream17]

I've gone ahead and rebased 165 commits again to include #384

[confused-Techie]

The windows test had failed here, so I've rerun them otherwise, as before (with these new changes) tests are looking great!

• find-and-replace: 47 Failures ✅
• settings-view: 1 Failures ❌ (Not an issue on this PR, we are seeing this repo wide)
• symbols-view: 2 Failures ✅
• tree-view: 2 Failures ✅

So with how long we've had to sit on this on, and the fact that nobody has found any issues with it, how do we @pulsar-edit/core how do we feel about merging this one? And if anything I can merge at the end of the day, if that sounds alright.

[icecream17]

Oop, that latest mention said that optional chaining is illegal in the left hand side of an assignment. I don't think I implemented that; should probably add tests...

[icecream17]

unfortunately, the general pattern a?.prop = 7 isn't checkable without massive work so I think after I add tests this pr is ready. i'm not completely sure that the ternary expression true?.5:1 parses either

another thing a future pr might do is update the picture in README.md since now ?? should be syntax highlighted

Edit: The commit before the merge failed one test and I'm not sure why, so merging 334 commits.

[confused-Techie]

@icecream17 Do those last few commits addressed the issue you pointed out above?

Since right now all tests but the windows build (which is failing for unrelated reasons) is looking good, and I'd be inclined here to get this merged so you can stop worrying about it, and we can make use of your awesome contributions here

[icecream17]

yes, my concerns are resolved

[confused-Techie]

Just want to say @icecream17 I really do appreciate your efforts here. Sorry they have taken so long to fully address, but it's awesome that you've stayed up to date here, and keeping this one working.

I'll go ahead and merge this one, appreciate you