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

[Addon Dev] allow all of rollup's OutputOptions to be passed to addon.output() #1104

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Feb 3, 2022

Update!

This PR adds pass-through support for all of rollup's output options:

output: addon.output({ sourcemap: true });

Intellisense confirmed working, too:
image


(old PR description)

I found myself wanting to provide sourcemaps for libraries.
addon.output() does not allow for that, but you could do:

output: {
  ...addon.output(),
  sourcemap: true,
}

but, that's extra code on the consuming side, when it could just be 1 extra line of config in the new Addon part of the rollup config file.

There are a lot of output options tho: https://rollupjs.org/guide/en/#outputoptions-object.
🤷

Maybe instead, we can take any object as an argument to .output() and merge that in the rollup config?

@NullVoxPopuli NullVoxPopuli force-pushed the addon-dev-passthrough-sourcemaps branch from b8def3e to 635bbfd Compare February 3, 2022 20:35
@NullVoxPopuli
Copy link
Collaborator Author

I removed sourcemap from the constructor, kept the docs tho, and added all output options to the output function. Looks kinda fun (intellisense test):

image

kinda weird they don't have any description or anything on these, but that's work we PR to rollup to help everyone out

@NullVoxPopuli NullVoxPopuli changed the title [Addon Dev] Pass sourcemap through, and add JSDoc for the Addon options for editor hints a… [Addon Dev] allow all of rollup's OutputOptions to be passed to addon.output() Feb 4, 2022
@ef4
Copy link
Contributor

ef4 commented Feb 5, 2022

This PR comes down the difference between:

{
  // current
  output: { ...addon.output(),  sourceMap: true }
}

and

{
  // proposed
  output: addon.output({ sourceMap: true })
}

They're extremely close, but the proposed option forces you to read the documentation for addon.output() to understand what's going to happen, whereas the current option is Just Javascript™️ and it's transparent what's happening. So I'm not a fan.

@NullVoxPopuli
Copy link
Collaborator Author

aight, sounds good! 🥳

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.

3 participants