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

WIP: support for class fields #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nathancahill
Copy link
Collaborator

@nathancahill nathancahill commented Nov 4, 2018

Solves #123.

This is just a start. Todo:

  • Regular fields x = 0
  • Bare fields x
  • Computed fields [x] = 0
  • Bare computed fields [x]
  • Does not remove surrounding whitespace

Feedback and pointers on removing the whitespace (does Buble generally do that or leave it?) See the whitespace in the tests.

Additional tasks by @adrianheine:

  • Add support data
  • Throw error on attempted private field transpilation
  • Bare string literal fields "ab"
  • String literal fields with value "ab" = 0
  • Don't throw on two fields with newline (class {x\ny})

return;
}
});

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's possible to use move here instead of removing the text and inserting it somewhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure it's possible to do this, since we're creating the constructor as a string, not a magic string. Right now the logic is collect fields => join fields with newlines => insert into top of constructor. To do this with move (besides converting constructor to magic string), we'd have to loop backwards (so we can always append to the front) or track the end position of the last inserted field.

Copy link
Member

@adrianheine adrianheine Nov 5, 2018

Choose a reason for hiding this comment

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

Yeah, it's probably complicated if possible at all. Let's leave it as it currently is and deal with issues once they arise. Maybe it will be possible to move the super call to the front, that should fix a lot of issues.

@nathancahill
Copy link
Collaborator Author

Wondering about private fields, if they should be included in this PR. The current spec is interesting https://tc39.github.io/proposal-class-fields/#sec-private-names

@adrianheine
Copy link
Member

I'm definitely willing to merge this without private fields if they are considerably more work. You could use acorn-private-methods to parse them, if you want to try it. There's also acorn-static-class-features, but I think we should get this with the current scope merged :)

@nathancahill
Copy link
Collaborator Author

Removing the private class fields todo for now. Your acorn-class-fields does support them, but it'll be complex to implement it in this PR, so we'll leave it for later (maybe with private methods too).

Last remaining item is the whitespace, does Buble remove it in other places when code is fully removed from a line leaving the line blank?

@adrianheine
Copy link
Member

Since we parse private fields, but don't transpile them, we should throw an error until we support them.

@adrianheine
Copy link
Member

Could you add the package-lock.json changes, too?

@adrianheine
Copy link
Member

I think for now I'm okay with the whitespace as it is.

@nathancahill
Copy link
Collaborator Author

Done, but not sure why it's failing in Node 4 and 6.

@nathancahill
Copy link
Collaborator Author

I think it might be a parser error?

       classes-no-named-function-expressions.js
         transpiles a subclass with super calls:
     SyntaxError: super() call outside constructor of a subclass (4:5)
  1 : 
  2 :       class Foo extends Bar {
  3 :         constructor ( x ) {
  4 :           super( x );
                ^

It seems right, the super call is inside the constructor.

@adrianheine
Copy link
Member

It's leaving stray semicolons:

class X { constructor() {} m = 5; y = 4; }
var X = function X() {
	this.m = 5;
	this.y = 4;}; ; 

@nathancahill
Copy link
Collaborator Author

Ah, thanks! How'd you see that? And how can I reproduce that to fix it?

@adrianheine
Copy link
Member

Yeah, I don't know about the node 4/6 issues either. I'll look into it. As for the semicolons, the first snippet is the input, the second the output.

@nathancahill
Copy link
Collaborator Author

Took a swing at the semicolon issue by removing everything up to the next statement. Doesn't work because it would trim comments. Going to rework it to loop until hitting the next character, if it's a ; remove it, otherwise stop.

@nathancahill
Copy link
Collaborator Author

Here's another approach that handles the semicolons but leaves the comments. This might not actually be as nice, since the comment is left dangling where the class fields were pulled from.

@nathancahill
Copy link
Collaborator Author

nathancahill commented Nov 6, 2018

Here's the outputs of the two options:

Input

class X {
    constructor() {}
    m = 5;
    // comment
    y = 4;
}

Output trim until semicolon:

var X = function X() {
    this.m = 5;
    this.y = 4;};

// comment

Output trim until next statement:

var X = function X() {
    this.m = 5;
    this.y = 4;}; 

@nathancahill
Copy link
Collaborator Author

nathancahill commented Nov 6, 2018

I don't think other parts of Buble move code around like classes does, so this might be new territory as far as deciding what to do about stray comments.

Babel leaves the comment dangling:

var X = function X() {
    _classCallCheck(this, X);

    this.m = 5;
    this.y = 4;
}
// comment
;

@adrianheine
Copy link
Member

I 'm wondering if it's not possible to leave the body where it is and turn the class around it into a function and move the methods out of the function?

@nathancahill
Copy link
Collaborator Author

You'd still end up having to move some code. To me, the "least surprising" code movement is the class fields to the constructor rather than the methods.

@adrianheine
Copy link
Member

Could you rebase this and maybe already squash the commits? I just added running bublé over the test262 tests to travisci and I'd like to see the result of that :)

@nathancahill nathancahill force-pushed the classfields branch 2 times, most recently from 31d5848 to 488a343 Compare November 19, 2018 03:45
@nathancahill
Copy link
Collaborator Author

Rebased and squashed. Will look in to test262 soonish.

@greypants
Copy link

Excited about this work! We're using react-live for our component docs, and getting this feature in will remove a bunch of boilerplate from our examples.

@adrianheine
Copy link
Member

I rebased this, added some data about failing test262 tests and added 5 tasks to the top post which I think need to be done before we can merge.

@yeliex
Copy link

yeliex commented Jul 3, 2019

any progress?

@trusktr
Copy link
Contributor

trusktr commented Jun 8, 2020

Come on Svelte! Where you at?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants