bublejs / buble

https://buble.surge.sh
MIT License
871 stars 67 forks source link

WIP: support for class fields #169

Open nathancahill opened 6 years ago

nathancahill commented 6 years ago

Solves #123.

This is just a start. Todo:

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:

nathancahill commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

adrianheine commented 5 years ago

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

adrianheine commented 5 years ago

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

nathancahill commented 5 years ago

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

nathancahill commented 5 years ago

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 commented 5 years ago

It's leaving stray semicolons:

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

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

adrianheine commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Rebased and squashed. Will look in to test262 soonish.

greypants commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

any progress?

trusktr commented 4 years ago

Come on Svelte! Where you at?