Allow injecting Acorn plugins #1857

stasm · 2018-01-09T17:25:27Z

This PR makes it possible to pass Acorn plugins into Rollup.

Acorn doesn't support non-Stage 4 proposals. There is a number of plugins which add support for the upcoming ECMAScript features. Inspired by comments in #1512 I'd like to propose to make it possible to add plugins to Acorn via rollup.config.js.

I added a new optional field in Rollup input options called acornInjects. If specified, it should be an array of inject functions which hook plugins into Acorn.

Example rollup.config.js:

import acornAsyncIteration from 'acorn-async-iteration/inject';

export default {
    // … other options …
    acorn: {
        ecmaVersion: 9,
        plugins: { asyncIteration: true }
    },
    acornInjectPlugins: [
        acornAsyncIteration
    ]
};

I originally wrote this for my own use. I'm submitting this PR to see if you think it's a good addition. I'll be happy to add tests to this PR if you do.

guybedford

Would be great to have this feature, thanks for the PR.

guybedford · 2018-01-09T18:03:55Z

src/Module.ts

@@ -45,9 +45,10 @@ export interface CommentDescription { block: boolean, text: string, start: numbe
 export interface ExportDescription { localName: string, identifier?: string }
 export interface ReexportDescription { localName: string, start: number, source: string, module: Module }

-function tryParse (module: Module, acornOptions: Object) {
+function tryParse (module: Module, acornOptions: Object, acornInjects: Function[]) {


These injections don't need to happen for every module parse - rather the acornInjects.reduce(acc, plugin) => plugin(acc)) step can be done in the Graph constructor I think rather?

Acorn plugins are not composable in that they expect the acorn object to be passed as an argument (example). I think I'll need to import acorn in Graph.js, produce the final acorn object there, and then pass it to tryParse. Sound good?

Sounds sensible! Alternatively because we have live bindings in ES Modules, it could be possible to have Graph.ts explicitly export its own acorn variable which is imported by Module.ts.

Actually that said, much better to have class-level encapsulation actually here.

I wasn't able to figure out how to pass the entire acorn object to tryParse. I think it's related to the fact that it's declared as a namespace in @types/acorn, but I don't know TypeScript well enough to know for sure.

I ended up passing the parse function instead.

guybedford · 2018-01-09T18:04:11Z

src/rollup/index.ts

@@ -61,6 +61,7 @@ export interface InputOptions {
 	};

 	acorn?: {};
+	acornInjects?: Function[];


How about just calling this acornPlugins?

I'm happy to change the name as you see fit. I chose acornInjects to avoid the confusion between acorn.plugins (where plugins are enabled, e.g. asyncIteration: true) and acornPlugins (where plugin functions would be passed).

Hmm perhaps acornInjectPlugins to be clearer then?

I renamed this field to acornInjectPlugins. I can also rename it to acronPlugins if you prefer; perhaps it's not really all that confusing. I'll leave the final decision to you. Thanks.

stasm · 2018-01-10T15:38:06Z

I added a few tests. I wasn't quite sure where to put them, so I went for test/misc :)

stasm · 2018-01-10T15:40:46Z

@guybedford This is ready for your review. Once approved, I'd like to squash this into a single commit and update the commit message with the new name for acornInjects. For now, I've left all commits separate to make it easier to review the changes made since yesterday.

guybedford · 2018-01-10T16:20:18Z

@stasm looks great to me and thanks for padding out the tests.

There's on issue with the internal plugin we're applying for dynamic import - the line at https://github.com/rollup/rollup/pull/1857/files#diff-356410863c3a18aee7d09896d52d6b08R1 should be possible to be removed since Acorn is passed through, and also the line at https://github.com/rollup/rollup/pull/1857/files#diff-356410863c3a18aee7d09896d52d6b08R41 should be able to be moved into Graph.ts to be the first plugin that is attached when dynamic import is enabled.

Let me know if that makes sense, or if you have any questions on this further.

guybedford · 2018-01-10T16:20:57Z

Actually I see that the acorn import is stilll used for the type in the Module.ts file. Perhaps just alter this to import { IParse } from 'acorn' instead?

stasm · 2018-01-10T17:38:42Z

Yeah, I wasn't sure what wrapDynamicImportPlugin was doing. I'll try to make the changes you suggested, thanks!

stasm · 2018-01-10T17:46:44Z

If I remove https://github.com/rollup/rollup/pull/1857/files#diff-356410863c3a18aee7d09896d52d6b08R41, what should happen to https://github.com/rollup/rollup/pull/1857/files#diff-356410863c3a18aee7d09896d52d6b08L184 and https://github.com/rollup/rollup/pull/1857/files#diff-356410863c3a18aee7d09896d52d6b08L187? I don't think I understand what dynamicImport does.

stasm · 2018-01-10T17:52:30Z

Nevermind, Github didn't do a good job showing me the lines you linked to. I think I understand, let me try.

stasm · 2018-01-11T11:44:12Z

To the best of my understanding setModuleDynamicImportsReturnBinding still needs to be called in Module.ts because it's used to bind the Module's dynamicImports array to the plugin. I exposed it on the Graph instance so that at least the wrapping is done in Graph.ts.

guybedford

Thanks for adding the extra code. This looks great to merge to me.

stasm · 2018-01-11T13:48:44Z

I squashed the commits and rebased on top of the current master. Thanks for helping to get this ready to merge and for the review!

guybedford · 2018-01-11T15:11:50Z

Right after the last comment I ended up working on this - 4d97152, which alters this dynamic import application process.

So depending on which lands first we'll just need to adjust the conflict here.

lukastaegert · 2018-01-12T07:23:09Z

Sorry, missed this one for the latest release. Will probably release this together with #1841 once I have reviewed that. Thanks!

lukastaegert · 2018-01-12T07:23:43Z

You probably want to rebase to latest master

Add a new field in Rollup input options: `acornInjectPlugins`. If specified, it should be an array of `inject` functions which hook plugins into Acorn. Example `rollup.config.js`: ```js import acornAsyncIteration from 'acorn-async-iteration/inject'; export default { acorn: { ecmaVersion: 9, plugins: { asyncIteration: true } }, acornInjectPlugins: [ acornAsyncIteration ] }; ```

stasm · 2018-01-12T11:01:44Z

Rebased. @guybedford you might want to take another look. Your changes in #1816 are a bit over my head :) I'm still moving wrapDynamicImportPlugin(acorn); to Graph.ts after the rebase; is that cool?

guybedford · 2018-01-12T11:24:05Z

src/Graph.ts

+		this.acornParse = ensureArray(options.acornInjectPlugins).reduce(
+			(acc, plugin) => plugin(acc),
+			acorn
+		).parse;


Ideally we could add the dynamic import plugin via something like [...(this.dynamicImport ? [injectDynamicImportPlugin] : []) , ...ensureArray(options.acornInjectPlugins)].reduce(..., altering wrapDynamicImport to retun the acorn instance to match the pattern.

Not having the dynamicImport check on the plugin is my oversight though, no pressure to add that here you've done a lot already.

Yeah, I was thinking yesterday that it would be nice to use the same system to inject both internal and user-defined plugins. With your recent changes, it should be easy to do. Let me try.

Done in d58553b. I wasn't sure how to declare the return type of injectDynamicImportPlugin, but leaving it undeclared worked too.

guybedford · 2018-01-12T11:24:33Z

LGTM.

…are.

guybedford · 2018-01-12T12:00:07Z

👍 perfect!

stasm · 2018-01-12T12:06:32Z

Thanks :)

I wasn't sure what you meant by:

Not having the dynamicImport check on the plugin is my oversight though, no pressure to add that here you've done a lot already.

Would you like the plugin to check the value of acornOptions.plugins.dynamicImport? If so, I don't think it's needed. Looking at the source, Acorn uses this object to enable plugins, but plugins don't use it in any way.

guybedford · 2018-01-12T12:12:19Z

I was referring to only wrapping the injection with dynamic import enabled, exactly as you did in https://github.com/rollup/rollup/pull/1857/files#diff-7c1e6304262c178e7fe268b62060e26bR166.

stasm · 2018-01-12T12:14:24Z

Ah, I see. Cool, I think this is ready to merge, then! It might make sense to keep it as two separate commits: the first one adds a new feature and the second one moves an existing feature to use the new one. Feel free to squash if you prefer, though.

lukastaegert

Very nice!

stasm · 2018-01-22T08:00:22Z

@lukastaegert How would you like to proceed wrt. the merge conflicts? Should I try to rebase on top of release-0.55.0 or can we simply merge it into my branch first?

lukastaegert · 2018-01-22T10:16:52Z

It's ok if you just merge into your branch. I will use merges anyway for the final assembly of the release as there are very many different PRs that need to be consolidated. Thanks!

stasm mentioned this pull request Jan 9, 2018

Add mapContextAsync projectfluent/fluent.js#125

Merged

6 tasks

stasm force-pushed the acorn-injects branch from 6f3f2dc to f21c40c Compare January 9, 2018 17:51

guybedford requested changes Jan 9, 2018

View reviewed changes

guybedford mentioned this pull request Jan 9, 2018

Arrow functions in es6 class #1448

Closed

stasm force-pushed the acorn-injects branch 2 times, most recently from 8f63cd8 to 34b2022 Compare January 10, 2018 12:28

stasm force-pushed the acorn-injects branch from 3a356ab to 4ec016f Compare January 11, 2018 11:37

guybedford approved these changes Jan 11, 2018

View reviewed changes

stasm force-pushed the acorn-injects branch from 4ec016f to e9c8d5e Compare January 11, 2018 13:42

stasm force-pushed the acorn-injects branch from e9c8d5e to 9df5ce6 Compare January 12, 2018 10:43

guybedford mentioned this pull request Jan 12, 2018

Dynamic import hook #1790

Closed

guybedford reviewed Jan 12, 2018

View reviewed changes

Register the dynamic import plugin the same way user-defined plugins …

d58553b

…are.

lukastaegert mentioned this pull request Jan 16, 2018

Support ES.Next object rest spread in config files like rollup.config.js #1759

Merged

adrianheine mentioned this pull request Jan 18, 2018

Switch to acorn-dynamic-import plugin #1891

Merged

lukastaegert approved these changes Jan 21, 2018

View reviewed changes

lukastaegert changed the base branch from master to release-0.55.0 January 21, 2018 20:51

lukastaegert added this to the 0.55.0 milestone Jan 21, 2018

Merge branch 'release-0.55.0' into acorn-injects

4a70558

lukastaegert merged commit 4a70558 into rollup:release-0.55.0 Jan 23, 2018

stasm deleted the acorn-injects branch January 23, 2018 12:17

guybedford mentioned this pull request Jan 24, 2018

Request: Supporting Acorn JSX #1512

Closed

Allow injecting Acorn plugins #1857

Allow injecting Acorn plugins #1857

Conversation

stasm commented Jan 9, 2018 • edited Loading

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Jan 10, 2018

stasm commented Jan 10, 2018

guybedford commented Jan 10, 2018

guybedford commented Jan 10, 2018

stasm commented Jan 10, 2018

stasm commented Jan 10, 2018

stasm commented Jan 10, 2018

stasm commented Jan 11, 2018

guybedford left a comment

Choose a reason for hiding this comment

stasm commented Jan 11, 2018

guybedford commented Jan 11, 2018

lukastaegert commented Jan 12, 2018

lukastaegert commented Jan 12, 2018

stasm commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Jan 12, 2018

guybedford commented Jan 12, 2018

stasm commented Jan 12, 2018

guybedford commented Jan 12, 2018

stasm commented Jan 12, 2018 • edited Loading

lukastaegert left a comment

Choose a reason for hiding this comment

stasm commented Jan 22, 2018

lukastaegert commented Jan 22, 2018

stasm commented Jan 9, 2018 •

edited

Loading

stasm commented Jan 12, 2018 •

edited

Loading