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

Hooks #742

Merged
merged 2 commits into from
Jun 23, 2016
Merged

Hooks #742

merged 2 commits into from
Jun 23, 2016

Conversation

Rich-Harris
Copy link
Contributor

This implements hooks as discussed in #353, with a couple of differences – the result of bundle.generate(...) is unchanged, and ongenerate hooks can't do anything asynchronously (well, they can, but Rollup won't wait for them).

The onwrite hook can return a promise. Both it and ongenerate are passed a copy of the options object with an additional bundle property (the same object that rollup.rollup(...) resolves with). onwrite handlers are called in sequence, not in parallel.

I'm a little unsure about the naming of onwrite, since it's weird for an event handler to be asynchronous. Am open to alternative suggestions.

Feedback welcome!

@tivac
Copy link
Contributor

tivac commented Jun 23, 2016

Very excited to be able to handle https://github.com/tivac/modular-css/blob/master/src/rollup.js#L61-L84 in a more sensible way!

@Rich-Harris Rich-Harris merged commit 89329f4 into master Jun 23, 2016
@Rich-Harris Rich-Harris deleted the hooks branch June 23, 2016 18:06
@TrySound TrySound mentioned this pull request Jun 23, 2016
@tivac
Copy link
Contributor

tivac commented Jul 6, 2016

@Rich-Harris What's the intended difference between onwrite and ongenerate? How should a plugin use them?

It makes sense to me that onwrite should just go ahead and write to the file system, but I'm not sure how ongenerate should function. Since the return value is already set and not available to the actual plugins I don't see how a plugin returns anything useful from ongenerate.

Should it stick new properties onto the bundle?

For context, I'm trying to update the modular-css rollup plugin to use these new hooks instead of the current footer hack.

Once we've got it hashed out I can update the wiki page as well.

@TrySound
Copy link
Member

TrySound commented Jul 6, 2016

I don't see a sense in onwrite hook which will never be called with custom writing on disk. Ongenerate will be called in any case.

@tivac
Copy link
Contributor

tivac commented Jul 6, 2016

WIP usage of ongenerate here: tivac/modular-css@f2fc437

After thinking about it more I'm with @TrySound, I don't see an obvious use for onwrite w/ the current API. I'm using ongenerate to write out to disk to ensure it happens no matter if the user calls bundle.generate() or bundle.write().

If there's a standardized way to return data that the user can access from ongenerate I'd split them up though, because writing to disk during bundle.generate() feels dirty.

@Rich-Harris
Copy link
Contributor Author

Yeah, I see where you're coming from.

If there's a standardized way to return data that the user can access from ongenerate

Any thoughts on what that should look like? I'm reluctant to make bundle.generate(...) asynchronous – would that pose difficulties?

@tivac
Copy link
Contributor

tivac commented Jul 7, 2016

Could the signature of ongenerate & onwrite change to include the rendered variable as the second argument?

plugin.ongenerate( assign({
    bundle: result
}, options ), rendered);

so that the plugin's ongenerate method could then add properties to the output object returned by calling bundle.generate()? It being sync is a bit of an issue for modular-css, but I could stick the promise onto the object and that'd be ok.

So modular-css's rollup plugin would do something like this:

ongenerate: function(options, rendered) {
    // processor.output() returns a promise, but that isn't the end of the world
    rendered.css = processor.output();
},

onwrite: function(options, rendered) {
    return rendered.css.then(function(result) {
        // write to disk blah blah
    });
}

Rollup's .write() method would need a tiny bit of refactoring to not use object destructuring so it could pass the entire result object to onwrite, but that'd be a small tweak I think.

Thoughts?

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

3 participants