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

Expose parse function to plugins #1945

Merged
merged 2 commits into from
Feb 15, 2018

Conversation

unstubbable
Copy link
Contributor

@unstubbable unstubbable commented Feb 5, 2018

This is motivated by #1512 and basically implements what @Rich-Harris was suggesting a few weeks ago.

By exposing rollup's parse function to plugins (via the transform context) these plugins can use the same acorn configuration (including injected plugins) if they need to operate on an ast. The most prominent example for this is probably rollup-plugin-commonjs, as mentioned in #1512. I opened a PR there as well to switch to the parse method of the transform context.

This adds a parse method to the transform context.
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Very nice! Also love the test! Found one minor comment, otherwise this is good to go!

@@ -51,7 +51,7 @@ function generateChunkName (id: string, chunkNames: { [name: string ]: boolean }
}

export default class Graph {
acornOptions: any;
acornOptions: acorn.Options;
Copy link
Member

Choose a reason for hiding this comment

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

👍

src/Module.ts Outdated
onComment: (block: boolean, text: string, start: number, end: number) =>
module.comments.push({ block, text, start, end }),
preserveParens: false
module.comments.push({ block, text, start, end })
}, acornOptions));
Copy link
Member

Choose a reason for hiding this comment

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

I think onComment should actually be added last as we are relying internally on this not being overwritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in cd53741

@lukastaegert lukastaegert added this to the 0.56.0 milestone Feb 8, 2018
@guybedford
Copy link
Contributor

Is there a way for this function to share the Rollup cache at all? Otherwise the internal parsing mechanism remains "privileged" in terms of providing this.

@unstubbable
Copy link
Contributor Author

unstubbable commented Feb 12, 2018

@guybedford I tried to investigate this. Maybe you can help me to understand the question first.

Are you referring to
a) the cachedModules of a Graph or
b) the ast of a Module or
c) something else entirely?

Depending on the answer I will probably have follow-up questions. 🙂

@unstubbable
Copy link
Contributor Author

Ahh, I just saw your reference to this issue in #1371. So you are probably talking about b) the ast of a Module.

In this case my follow-up question is this:

Right now the ast is stored in the Module after the source has been transformed by the plugins. So if I understand this correctly the "cache" currently only works in the other direction, i.e. the plugin can return the ast and rollup then does not need to parse again, right? Should this be somehow reversed or what was your idea?

@unstubbable
Copy link
Contributor Author

Oops, I guess the aforementioned order is irrelevant when you're talking about caching across builds in watch mode. But in this case the cached result will be returned before transform is even called, right? ... Sorry for my rambling, I don't have the full picture yet.

@guybedford
Copy link
Contributor

@KingHenne yes I'm referring to across builds. It sounds like you may be right in that it would only apply if a plugin for file x happens to parse file y's ast when file y has previously been transformed. Also not completely clear on how the cache layer works myself, just trying to work out the repercussions for future caching architectures of this change.

@lukastaegert lukastaegert merged commit cd53741 into rollup:master Feb 15, 2018
@unstubbable unstubbable deleted the transform-plugin-parse branch February 15, 2018 08:43
@tunnckoCore
Copy link
Contributor

Wouldn't it be better to expose the AST too? So there will be only one parsing... and plugins can work on it directly.

@unstubbable
Copy link
Contributor Author

@olstenlarck See discussion above.

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.

None yet

4 participants