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

Private method support #708

Closed
lgarron opened this issue Jun 25, 2021 · 8 comments · Fixed by #710 or #858
Closed

Private method support #708

lgarron opened this issue Jun 25, 2021 · 8 comments · Fixed by #710 or #858
Labels
bug Something isn't working

Comments

@lgarron
Copy link

lgarron commented Jun 25, 2021

Describe the bug

Private methods in classes seem to break wmr

To Reproduce
Steps to reproduce the behavior:

Add this code to e.g. the default template:

class foo {
  #hi() { console.log("hi"); }
  yo() { this.#hi(); }
}
new foo().yo();

This results in:

500 ./public/index.ts - Unexpected token (unknown:9:3)

   7 | 
   8 | class foo {
>  9 |   #hi() {
     |   ^
  10 |     console.log("hi");
  11 |   }
  12 |   yo() {

  at z_ (/Users/lgarron/Downloads/wmr-test/node_modules/wmr/wmr.cjs:2:958314)
  at Object.transform (/Users/lgarron/Downloads/wmr-test/node_modules/wmr/wmr.cjs:2:2521954)
  at Object.transform (/Users/lgarron/Downloads/wmr-test/node_modules/wmr/wmr.cjs:2:2841421)
  at async js (/Users/lgarron/Downloads/wmr-test/node_modules/wmr/wmr.cjs:2:2880063)
  at async Array.<anonymous> (/Users/lgarron/Downloads/wmr-test/node_modules/wmr/wmr.cjs:2:2877684)

Expected behavior

The code works.

Desktop (please complete the following information):

npm 7.15.0
node v16.2.0
macOS 11.4
wmr 3.1.0 (although npx wmr --version prints wmr, 0.0.0)

Additional context

I previously filed an issue at #255 when I was last evaluating wmr but it didn't support private fields. Private fields seem to work now, but private methods don't. The relevant upstream change at alangpierce/sucrase#560 (pulled in by #428) suggests to me that private methods should have been supported in wmr as soon as private fields were. I'm not sure what I'm missing here, but this would help unblock using wmr for a fairly major project.

@lgarron lgarron added the bug Something isn't working label Jun 25, 2021
@lgarron
Copy link
Author

lgarron commented Jun 25, 2021

(For what it's worth, I'm also running into other missing recent ES/TS features like ??=. But those are much easier to work around for now, whereas private methods are an important part of giving our library clients exactly the intended API.)

EDIT: In fact, I've done a more thorough test, and it seems private method support is the only real blocker. Parcel and Snowpack have some correctness bugs that are impossible for us to work around at this point, so I'd really love to try wmr.

@marvinhagemeister
Copy link
Member

Thanks for filing an issue! Sounds like we missed a few plugins for recent JS features. I've made a PR for private class fields. Will try to go through other syntax additions in the next week 👍

@lgarron
Copy link
Author

lgarron commented Jun 27, 2021

Thanks for filing an issue! Sounds like we missed a few plugins for recent JS features. I've made a PR for private class fields. Will try to go through other syntax additions in the next week 👍

woo Thanks for the fix, I was able to use a local wmr build (from the lovely instructions) to confirm it works.

@lgarron
Copy link
Author

lgarron commented Jul 28, 2021

Thanks for filing an issue! Sounds like we missed a few plugins for recent JS features. I've made a PR for private class fields. Will try to go through other syntax additions in the next week 👍

woo Thanks for the fix, I was able to use a local wmr build (from the lovely instructions) to confirm it works.

So, using npm install wmr@latest I get wmr version 3.4.1, but it's still failing on #private methods like before.
Even explicitly installing acorn-private-methods doesn't seem to do anything. Is it possible that the published version isn't working properly?

(Unfortunately, it's hard for me to look for a more direct repro, because npm init wmr project-name is not working for me.)

@lgarron
Copy link
Author

lgarron commented Jul 28, 2021

Even explicitly installing acorn-private-methods doesn't seem to do anything. Is it possible that the published version isn't working properly?

I forgot, wmr is compiled to one file, so this is not relevant.

https://unpkg.com/wmr@3.4.1/wmr.cjs shows that parsePrivateClassElementName is in the compiled code, not sure why it's not working as expected.

@marvinhagemeister
Copy link
Member

Reopening, so that this doesn't get lost.

@lgarron
Copy link
Author

lgarron commented Jul 28, 2021

Hmm, so I did some more debugging, and using wmr in a fresh project seems to work properly.
It doesn't seem that the parsePrivateClassElementName() method is ever invoked, though.

My leading theory now is that this is related to an issue where npm install has serious issues for me:
npm/cli#3581
That doesn't seem like a satisfying explanation because the project-level node_modules/wmr/wmr.cjs file is properly invoked and it's only a single file... but who knows.

@lgarron
Copy link
Author

lgarron commented Jul 28, 2021

This is my current repro:

git clone https://github.com/cubing/cubing.js; cd cubing.js
git checkout wmr2
npm install
npx wmr --version # verify that we're on `wmr` 3.4.1
npx wmr
open http://localhost:8080/src/sites/experiments.cubing.net/cubing.js/twisty/twisty-player.html

This results in:

500 /@alias/src/cubing/alg/traversal.ts - Unexpected token (src/cubing/alg/traversal.ts:132:10)

  130 | // TODO: Test that inverses are bijections.
  131 | class Simplify extends TraversalDownUp {
> 132 |   static #newAmount(
      |          ^
  133 |     move,
  134 |     deltaAmount,
  135 |     options,

  at sb (/Users/lgarron/Downloads/cubing.js/node_modules/wmr/wmr.cjs:2:965063)
  at Object.transform (/Users/lgarron/Downloads/cubing.js/node_modules/wmr/wmr.cjs:2:2538620)
  at Object.transform (/Users/lgarron/Downloads/cubing.js/node_modules/wmr/wmr.cjs:2:2861109)
  at async js (/Users/lgarron/Downloads/cubing.js/node_modules/wmr/wmr.cjs:2:2900347)
  at async Array.<anonymous> (/Users/lgarron/Downloads/cubing.js/node_modules/wmr/wmr.cjs:2:2898054)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants