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

Class private properties #7555

Closed

Conversation

CodingItWrong
Copy link
Contributor

@CodingItWrong CodingItWrong commented Mar 12, 2018

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

Rebases #6120 off of latest master, and attempts to get tests passing.

Note: I need to do some more significant rebasing; the work from #6120 was applied to the old transform-class-properties folder, not the new proposal-class-properties folder.

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 12, 2018

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

@babel-bot
Copy link
Collaborator

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

@littledan
Copy link

Great to see this being picked up. Thanks for your hard work here!

It would be a good next step if you could get the test262 tests passing as well. These test against several things that were buggy in the previous PR (see my review of that PR for an explanation). You can run test262 tests against Babel by using test262-harness with the --babelPresets flag, based on a JS engine binary from jsvu. Please let me know on this thread if you need any help with the setup (cc @mathiasbynens ).

@CodingItWrong
Copy link
Contributor Author

thanks @littledan, that sounds good. after i finish correcting the rebase on this pr, the test262 suite will be my next step!

@@ -1,6 +1,6 @@
{
"name": "@babel/plugin-codemod-optional-catch-binding",
"version": "7.0.0-beta.40",
"version": "0.0.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to worry about the version, they are managed by lerna

helpers.classPrivateFieldKey = () => template.program.ast`
var id = 0;
export default function _classPrivateFieldKey(name) {
// Can we use a middle finger emoji?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha that would be so cool, but the comment is kinda useless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove my snark.

@CodingItWrong
Copy link
Contributor Author

@littledan ok, I could use a pointer on how to run test262. I tried this:

cd test262
test262-harness --babelPresets test/**/*.js

But I get the following error:

ERROR Error: Couldn't find preset "/Users/josh/.nvm/versions/node/v8.9.4/lib/node_modules/test262-harness/node_modules/babel-preset-test/harness/arrayContains.js" relative to directory "/Users/josh/apps/babel/test262"

Can you help me interpret what I should be doing to fix that error? Also, what should I do to run the tests against my local babel checkout?

@littledan
Copy link

@CodingItWrong The --babelPresets flag should have an argument passed to it, such as stage-0. To run against your local Babel checkout, from the directory that npm installed test262-harness into, use the npm link command.

@CodingItWrong CodingItWrong force-pushed the class-private-properties branch 2 times, most recently from 93391d2 to 288da8b Compare March 20, 2018 15:59
@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Class Fields labels Mar 31, 2018
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Mar 31, 2018
2 tasks
jridgewell added a commit to jridgewell/babel that referenced this pull request Apr 18, 2018
Yes, the output is uglier. But, this is necessary for me to refactor
`replaceSupers` for babel#7733, which is necessary for both babel#7555 and
babel#7553 (comment).

I'm still in the middle of cleaning up all this code. Don't expect
`transformClass` to survive much longer as it's written currently.
@littledan
Copy link

How is this PR going? Is there anything more I could do?

@CodingItWrong
Copy link
Contributor Author

@littledan it sounds like @jridgewell is intending to pick up the work in a sequence of smaller PRs like #7750. If I get a chance to contribute again, I'll follow the same sequence 👍

@jridgewell
Copy link
Member

@littledan: I'm slowly picking away at this through smaller PRs to fix classes.

Particularly, once I get #7750 in, I can work on #7733, which'll remove a few hundred lines from this PR.

@littledan
Copy link

Great, thanks @jridgewell !

jridgewell added a commit to jridgewell/babel that referenced this pull request Apr 22, 2018
Yes, the output is uglier. But, this is necessary for me to refactor
`replaceSupers` for babel#7733, which is necessary for both babel#7555 and
babel#7553 (comment).

I'm still in the middle of cleaning up all this code. Don't expect
`transformClass` to survive much longer as it's written currently.
jridgewell added a commit that referenced this pull request Apr 22, 2018
Yes, the output is uglier. But, this is necessary for me to refactor
`replaceSupers` for #7733, which is necessary for both #7555 and
#7553 (comment).

I'm still in the middle of cleaning up all this code. Don't expect
`transformClass` to survive much longer as it's written currently.
@billinghamj
Copy link

It looks like all those PRs are merged now (awesome work!) - so I guess the question now is: what are the remaining steps?

@jridgewell
Copy link
Member

Final step is the class-properties transform! Working on it now.

@jridgewell
Copy link
Member

Have an updated branch on my personal repo. Relevant commit for the transform is jridgewell@d004652 (at 177 fewer lines!). All that's needed are tests.

@vjpr
Copy link

vjpr commented May 6, 2018

Superceded by #7842

@hzoo
Copy link
Member

hzoo commented May 19, 2018

Closing via #7842 👍

@hzoo hzoo closed this May 19, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Class Fields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants