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

Transform await in computed class keys #14391

Merged
merged 11 commits into from Jun 27, 2022
Merged

Conversation

Yokubjon-J
Copy link
Contributor

@Yokubjon-J Yokubjon-J commented Mar 24, 2022

Q                       A
Fixed Issues? Fixes #14347
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Mar 24, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52376/

@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Mar 27, 2022

I have modified package.json files (of 2 packages) by adding the necessary dependencies and I think this is why build-standalone and other tests are failing (and I see The lockfile would have been modified by this install, which is explicitly forbidden. warning).
However, even after I have these modifications, the output file is still producing the same error.
I'd like to hear your thoughts about it.

@liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented May 29, 2022

It seems that you need to run the yarn command in the root directory and then commit the .lock file.

@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 18, 2022

@liuxingbaoyu wow I have noticed your comment after 20 days since you commented (unfortunately)!
Thank you very much, I am going to try it!

@nicolo-ribaudo nicolo-ribaudo changed the title Draft 14347 Fix yield/await in computed class keys Jun 19, 2022
@liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Jun 21, 2022

Can you do a rebase?
Also when you think it can be reviewed, please mark it as ready.

@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 22, 2022

I tried to rebase draft-14347 on main, but I am getting an error:

Auto-merging packages/babel-plugin-proposal-async-generator-functions/package.json
Auto-merging packages/babel-plugin-proposal-async-generator-functions/src/index.ts
CONFLICT (content): Merge conflict in packages/babel-plugin-proposal-async-generator-functions/src/index.ts
error: could not apply 6548c38378... environmentVisitor merged but issue still persists

To resolve the conflict, I ran git add . then git commit -am "commit all" but I got another error:
ESLint couldn't find the plugin "@babel/eslint-plugin-development".
I would like to know your advice on how to proceed forth.
Here is the screenshot of the last error in full:
image

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 22, 2022

We lint Babel using Babel itself, so you need to compile (make build) before committing. Or you can use git commit -am "commit all" --no-verify to skip linting.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 22, 2022

@Yokubjon-J It looks like something went wrong when rebasing, do you need any help?

@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 23, 2022

@nicolo-ribaudo Thank you, I would be very happy!
I wanted to update my branch (draft-14347) with the main branch while retaining my commits. But I got some conflicting commits and make build also threw errors. Some of the files were damaged like so; make bootstrap and make build stopped generating lib folder in babel-plugin-proposal-async-generator-functions.

@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 23, 2022

What I wanted to do with rebase is update my branch draft-14347 with main.

@liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Jun 23, 2022

Great, CI all passed.

@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 23, 2022

Oh I thank you very much @nicolo-ribaudo! It was so fast! I thought it would be a complicated process. Now I am going to work on the issue of the bug.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 23, 2022

@Yokubjon-J If you also need help with downloading the updated branch, you probably have to do something like this:

git fetch
git reset origin/draft-14347 --hard

@nicolo-ribaudo nicolo-ribaudo changed the title Fix yield/await in computed class keys Transform await in computed class keys Jun 24, 2022
@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 24, 2022

@Yokubjon-J The reason why it's not working is because we are still doing Function(path) { path.skip(); }. A class method is a function, so we are skipping the whole method (including its key). It's probably enough to just delete that Function visitor, since environmentVisitor will take care of that.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 24, 2022

Actually, we probably still need to manually skip ArrowFunctionExpression because environmentVisitor does not skip them.

@Yokubjon-J Yokubjon-J marked this pull request as ready for review Jun 25, 2022
@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 25, 2022

I think I am getting errors because I forgot to run make test, I am now running them and once complete I am going to push the changes.

…ndex.ts

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 25, 2022

Can you run yarn && yarn dedupe again and commit the updated yarn.lock file?

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Thanks!

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 label Jun 25, 2022
@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 25, 2022

@nicolo-ribaudo Thank you a lot! I have learned so much! Especially getting to know that class methods (inside classes) are treated as function methods was insightful!

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 27, 2022

Hey, no need to keep this up to date with main! I'm just waiti f for a second review.

@liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Jun 27, 2022

Generally, rebasing is only required after there have been a lot of changes in the main branch.
Also in PR we should use rebase instead of merge.

@Yokubjon-J
Copy link
Contributor Author

@Yokubjon-J Yokubjon-J commented Jun 27, 2022

@liuxingbaoyu I am afraid of rebasing for the fear of getting that problem again :)

@liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Jun 27, 2022

This is normal, I screwed everything up the first time I did the rebase.
If you use VS CODE, you can use the gitlens plugin, which is very convenient.

@nicolo-ribaudo nicolo-ribaudo merged commit 6347eaf into babel:main Jun 27, 2022
35 checks passed
@Yokubjon-J Yokubjon-J deleted the draft-14347 branch Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants