feat(parser): add for-await-of parsing #543
Conversation
| @@ -2632,14 +2632,17 @@ so many of its properties will be nil. | |||
| object | |||
| in-pos | |||
| each-pos | |||
| foreach-p forof-p | |||
| await-pos | |||
There was a problem hiding this comment.
await-pos is unused, isn't it? When printing, etc.
There was a problem hiding this comment.
Same could be said for each-pos, of course, but let's not worry about that.
There was a problem hiding this comment.
I can remove them both, I think they aren't necessary. But maybe some 3rd party depends on it, who knows.
| lp rp))) | ||
| "AST node for a for..in loop." | ||
| iterator ; [var] foo in ... | ||
| object ; object over which we're iterating | ||
| in-pos ; buffer position of 'in' keyword | ||
| each-pos ; buffer position of 'each' keyword, if foreach-p | ||
| await-pos ; buffer position of 'await' keyword, if forawait-p | ||
| foreach-p ; t if it's a for-each loop |
There was a problem hiding this comment.
We could also set foreach-p to a symbol (e.g. AWAIT), instead of adding a new field. But I'm okay with either.
There was a problem hiding this comment.
I did it this way to not possibly break existing integrations outside this package. If we're okay with removing backward compatibility we can use as symbol as you say.
Maybe in the future they will allow for each await () ? :D
There was a problem hiding this comment.
Integrations that use an (eq ... t) check? I wouldn't be too worried about those.
Would for each await actually make any kind of sense?
There was a problem hiding this comment.
Rather even (if foreach-p ...) which would now be set to a symbol even in the case it is not an each keyword. But of course we would rename the field so I guess in fact that mitigates the problem and should allow an easy migration path.
I'm honestly not even sure what for each does. So probably not, never used it in my life.
There was a problem hiding this comment.
Actually if we don't rename the field, it should make the "migration" easier: only dependents that know about "for await" would recognize it, and the rest would continue to compile and treat the AST nodes as "for each". Which is the outdated syntax.
It was Mozilla's extension. The modern counterpart is "for of": https://developer.mozilla.org/ru/docs/Web/JavaScript/Reference/Statements/for_each...in
There was a problem hiding this comment.
Again, I'm fine with either (rename or not). But reusing the field seems more economical.
|
Could you add a NEWS entry as well? |
|
@dgutov will do thanks for the feedback |
|
@Fuco1 May I ask the status of this? I would very much like if this gets merged... |
|
@pcr910303 The status is that I forgot about it 😊 @dgutov is the news entry the only thing blocking this? |
|
@Fuco1 Pretty much. The other two discussions don't seem critical. |
|
Jesus, please just merge this and add the NEWS entry in a separate commit. There's absolutely no need to be bureaucratic here. |
|
Okay. |
|
I don't know if it was that unreasonable to expect the contributor to do at least one revision after review, but oh well. |
|
It is unreasonable if you hold up perfectly good progress for over a year due to a missing NEWS entry that nobody reads and you can add yourself. |
|
Next time you have an idea about what somebody else should do, try helping. |
|
Help how? You are the maintainer, not me and I don't care about the NEWS entry. I commented, you got a notification, the code got merged after over a year. That's helping everyone. |
So rather than pitch in to maintain the project standards, you chose to be rude instead. Thanks for the reminder, but the rest could have been done better. |
|
I'll make sure I point out the obviously unreasonableness of holding up a perfectly good PR for over year next time. Without any exclamations involving any deities. I promise. |
Fixes #412