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

Structured code generation #3539

Merged
merged 95 commits into from
Oct 16, 2019
Merged

Structured code generation #3539

merged 95 commits into from
Oct 16, 2019

Conversation

Rich-Harris
Copy link
Member

This is completely experimental and may go nowhere.

One of the things that gets a bit frustrating when writing the Svelte compiler is that the code generation is all string-based. Creating new code is done with the CodeBuilder class and the deindent helper, both of which have all sorts of quirks, and manipulating user code is done with MagicString, which is also responsible for gluing everything together at the end, using weird, brittle kludges like [✄start-end✄] to preserve source mappings.

I have a hunch that our lives would be much easier if we were manipulating an AST and stringifying it, and that we'd have new opportunities for code optimisation.

The approach taken in this PR is to use a purpose built utility called code-red (shitty name, but codeine ('pain-free code generation!') and codify etc were all taken already. Alternatives welcome) which simplifies the process of generating AST nodes.

So far all I've managed to do is get everything to typecheck, it doesn't, like, do anything yet.

@Swatinem
Copy link
Member

Swatinem commented Sep 9, 2019

I have a hunch that our lives would be much easier if we were manipulating an AST and stringifying it, and that we'd have new opportunities for code optimisation.

❤️

I wonder if it is possible to structure this PR in a way that is more piecemeal and easier to review…

@Rich-Harris
Copy link
Member Author

I would love it if that were the case but I'm not sure how, the two approaches are rather incompatible. May need to engineer some kind of solution if the brute force approach turns out to be too confusing or difficult to finish

@antony
Copy link
Member

antony commented Sep 9, 2019

codeia is another word for codeine

@Rich-Harris
Copy link
Member Author

Reached a small but significant milestone — the first working component produced with this method. Behold, the results of compiling <h1>Hello world!</h1>:

import {element, insert, noop, detach, SvelteComponent, init, safe_not_equal} from "svelte/internal";;
function create_fragment(ctx) {
    let h1;
    return {
        c: function create() {
            h1 = element("h1");
            h1.textContent = "Hello world!";
        },
        m: function mount(target, anchor) {
            insert(target, h1, anchor);
        },
        p: noop,
        i: noop,
        o: noop,
        d: function destroy(detaching) {
            if (detaching) detach(h1);
        }
    };
}
class App extends SvelteComponent {
    constructor(options) {
        super();
        init(this, options, null, create_fragment, safe_not_equal, []);
    }
}
export default App;

The current output for the same input:

/* App.svelte generated by Svelte v3.12.1 */
import {
    SvelteComponent,
    detach,
    element,
    init,
    insert,
    noop,
    safe_not_equal
} from "svelte/internal";

function create_fragment(ctx) {
    var h1;

    return {
        c() {
            h1 = element("h1");
            h1.textContent = "Hello world!";
        },

        m(target, anchor) {
            insert(target, h1, anchor);
        },

        p: noop,
        i: noop,
        o: noop,

        d(detaching) {
            if (detaching) {
                detach(h1);
            }
        }
    };
}

class App extends SvelteComponent {
    constructor(options) {
        super();
        init(this, options, null, create_fragment, safe_not_equal, []);
    }
}

export default App;

@Rich-Harris
Copy link
Member Author

Quick update: this is laborious, but i'm getting there. Focusing on the tests in tests/runtime initially so as to make it manageable, and skipping the SSR stuff, and out of 669 tests, 571 are currently passing.

Some things are definitely harder when you can't just throw strings at the problem (e.g. you can't conditionally append an else block to a prior if block, because else {...} by itself won't parse). But overall I'm convinced that this is the right direction.

Didn't mention this above so I'll do it briefly here — my goal once everything is working is to add a Rollup plugin to Svelte's own build step that turns this...

const node = x`${foo} = ${bar}`;

...into something like this...

const node = {
  type: 'AssignmentExpression',
  operator: '=',
  left: foo,
  right: bar
};

...so there's no compile-time (or rather run-time, from Svelte's perspective — I guess the terminology gets a bit weird at this level of abstraction) penalty for generating ASTs.

@Rich-Harris
Copy link
Member Author

Not quite sure why the lint check is failing

@dasZGFz
Copy link
Contributor

dasZGFz commented Oct 14, 2019

Changing rollup.config.js#L80 like so fixed the lint error for me:

- fs.writeFileSync(`${dir}/index.d.ts`, `export * from '../types/runtime/${dir}/index';`);
+ fs.writeFileSync(`${dir}/index.d.ts`, `export * from '../src/runtime/${dir}/index';`);

@Conduitry
Copy link
Member

Conduitry commented Oct 14, 2019

Ohhhh I see. I already had the .d.ts types generated locally from when I last published, and presumably Rich did as well. The linting CI job did not because they're normally only run during prepublish. I can reproduce this by deleting my types folder.

One way to handle this would be to add npm run tsd to the lint job here, but that would still have the issue when people ran linting locally without having run npm run tsd at least once first. Another way would be to add npm run tsd to as part of npm run lint but that would produce unnecessary work and would make linting locally take longer.

If I use the versions of dependencies in this branch and then try to lint the master branch (with no local types built), I get these same failures, so they appear to have been triggered by just some dependency update, and not any other code changes here. I'm poking around to see whether I can find a nice way to handle all this.

Edit: Ohhhh part II. Moving the tsd job from during the build to during prepublish happened in this PR. I'd forgotten that wasn't already in master. Okay so that simplest change would probably be to just undo that change, and we can worry about making this speedier later, because it won't be any worse than what we have already.

Copy link
Member

@Conduitry Conduitry left a comment

Choose a reason for hiding this comment

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

The whole deindent util module can be removed now.

rollup.config.js Outdated Show resolved Hide resolved
@dasZGFz
Copy link
Contributor

dasZGFz commented Oct 15, 2019

I'm not sure if #3548 should be added to this PR or be added separately. I'm fine either way but thought it's worth mentioning.

@Rich-Harris Rich-Harris merged commit b9f1484 into master Oct 16, 2019
@Rich-Harris Rich-Harris deleted the code-red branch October 16, 2019 14:57
@Rich-Harris
Copy link
Member Author

@dasZGFz for sure — I've merged master into #3458. We may need to update the tests, will see what CI says

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.

7 participants