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
Conversation
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 To do that, you use the |
docs/05-plugins.md
Outdated
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. |
There was a problem hiding this comment.
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 tobundle.generate
- with my suggestion, the limitations can be removed
src/rollup/index.ts
Outdated
|
||
return outputOptions; | ||
} | ||
|
There was a problem hiding this comment.
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
src/rollup/index.ts
Outdated
: plugins | ||
? [plugins] | ||
: []; | ||
const outputOptions = inputOptions.plugins.reduce(applyOutputOptionHook, mergedOutputOptions); |
There was a problem hiding this comment.
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.
src/rollup/types.d.ts
Outdated
@@ -230,6 +230,10 @@ export interface Plugin { | |||
chunk: OutputChunk | |||
) => void | Promise<void>; | |||
options?: (this: MinimalPluginContext, options: InputOptions) => InputOptions | void | null; | |||
outputOptions?: ( | |||
this: MinimalPluginContext, |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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
I've implemented |
src/rollup/index.ts
Outdated
const outputOptionsReducer = (outputOptions: OutputOptions, result: OutputOptions) => { | ||
if (!result) return outputOptions; | ||
|
||
return {...outputOptions, ...result}; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
#2735
Description
This PR adds
outputOptions
hook for plugins, that allows overriding ofOutputOptions
passed tobundle.generate
. I've based the code on the existingoptions
hook. The hook is run insidenormalizeOutputOptions
, as it seemed to be the place, where the final form of output options is created.