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

Add hook for modifying OutputOptions #2736

Merged
merged 8 commits into from Mar 8, 2019
Merged

Add hook for modifying OutputOptions #2736

merged 8 commits into from Mar 8, 2019

Conversation

Comandeer
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

#2735

Description

This PR adds outputOptions hook for plugins, that allows overriding of OutputOptions passed to bundle.generate. I've based the code on the existing options hook. The hook is run inside normalizeOutputOptions, as it seemed to be the place, where the final form of output options is created.

@lukastaegert
Copy link
Member

Hi @Comandeer, this looks like a valuable addition. However I would propose to make this a "proper" hook using a plugin context which enables the hook to e.g. display warnings using Rollup's internal mechanism. The original options hook was a rather bad example to copy from because this hook cannot use the proper context as many fundamental things including plugins are not known before this hook has run.

To do that, you use the pluginDriver which is attached to the graph. As I think it makes sense to keep the hook synchronous, we would need a new hookReduceArg0Sync to handle this.

Type: `(outputOptions: OutputOptions) => OutputOptions | null`<br>
Kind: `sync, sequential`

Reads and replaces or manipulates the output options object passed to `rollup.rollup`. Returning `null` does not replace anything. This hook does not have access to most [plugin context](guide/en#plugin-context) utility functions as it is run before rollup is fully configured.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking care to update the documentation! Some comments:

  • output options are not passed to rollup.rollup but to bundle.generate
  • with my suggestion, the limitations can be removed


return outputOptions;
}

Copy link
Member

Choose a reason for hiding this comment

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

This can be handled by the pluginDriver

: plugins
? [plugins]
: [];
const outputOptions = inputOptions.plugins.reduce(applyOutputOptionHook, mergedOutputOptions);
Copy link
Member

Choose a reason for hiding this comment

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

This can be handled by the pluginDriver. To get access, you probably need to pass it to this function.

@@ -230,6 +230,10 @@ export interface Plugin {
chunk: OutputChunk
) => void | Promise<void>;
options?: (this: MinimalPluginContext, options: InputOptions) => InputOptions | void | null;
outputOptions?: (
this: MinimalPluginContext,
Copy link
Member

Choose a reason for hiding this comment

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

This can be the proper PluginContext

{
renderChunk(code, chunk, options) {
assert.strictEqual(options.banner, 'new banner');
assert.strictEqual(options.format, 'cjs');
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to verify that not only the options are updated, but the code was actually rendered using the updated options by inspecting the generated code.

For the latter, I think it would be best to inspect the output of bundle.generate

@Comandeer
Copy link
Contributor Author

I've implemented pluginDriver.hookReduceArg0Sync method and used it inside normalizeOutputOptions to run outputOptions hook. I've also updated docs and tests according to your suggestion.

const outputOptionsReducer = (outputOptions: OutputOptions, result: OutputOptions) => {
if (!result) return outputOptions;

return {...outputOptions, ...result};
Copy link
Member

Choose a reason for hiding this comment

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

The rest looks really good, this is the only thing giving me pause. I am not sure we should be merging the output options; at least when compared to the options hook that only replaces options. Though I would mostly argue for consistency here.

Also, this makes it impossible to remove options (though of course you could set them to undefined), which could be confusing for some options where undefined and false behave differently (e.g. output.interop).
And lastly, this is only a shallow merge which may or may not be what people expect for object valued options (there are only few, e.g. output.amd).

Would you be ok with a change or do you have strong arguments for merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't have any strong arguments. TBH I also had some doubts about it, yet it seemed like a more elegant solution to me. However you made good point about removing options. I simplified the code to just replace options.

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.

Great, thanks a lot!

@lukastaegert lukastaegert merged commit 00f414d into rollup:master Mar 8, 2019
@Comandeer Comandeer deleted the outputOptions-hook branch March 8, 2019 09:25
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

2 participants