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

feat: add meta property and rollupVersion to plugin context #2394

Merged
merged 10 commits into from
Aug 24, 2018

Conversation

shellscape
Copy link
Contributor

@shellscape shellscape commented Aug 10, 2018

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Please Describe Your Changes

This resolves #1374 by changing the signature of the options hook to accept a second parameter named meta which currently will contain { version: string }. by adding a meta property to a plugin context (this within hooks), which contains rollupVersion as suggested.

I ended up piggy backing on the test that checks the options hook can effect inputOptions. If there's a better place for that, I'm all for it. which checks validity of buildStart and buildEnd hooks. The filename is just plain old hooks, so it seemed an apt place for them. (This is probably a case of the tests needing a bit of spring cleaning.)

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this.

It might be more convenient to have this information available to all hooks through the plugin context - this.rollupVersion or similar perhaps?

@shellscape
Copy link
Contributor Author

I can definitely take a look at going that route. I'm a big fan of the meta property name to contain non-essential metadata. Any objection to that naming?

@guybedford
Copy link
Contributor

I'd be happy with that.

@shellscape shellscape changed the title feat: add meta/version to plugin.options feat: add meta property and rollupVersion to plugin context Aug 10, 2018
@shellscape
Copy link
Contributor Author

@guybedford changes made, tests passed locally. ESLint was responsible for the whitespace changes in hooks/index.js and they appear legitimate changes.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks good to me.

What else do you see this meta containing in future?

@@ -6,7 +6,7 @@ module.exports = {
options: {
plugins: [
{
options(options) {
options(options, meta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should no longer be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that sneaky bugger. Uno momento por favor.

@shellscape
Copy link
Contributor Author

Lot's of opportunity for hypotheticals, might include:

  • metrics of varying kinds (plugin run times, cache metrics, ...)
  • plugin info we could populate automatically, which could be used in future custom build reports/reporters

Mostly I've used meta to future-proof as a home for static bits of information, so as not to pollute (used in loose terms) a namespace or object properties. I'm not held strongly to using that and wouldn't push back on tacking rollupVersion straight onto the context, if meta doesn't sit right.

@guybedford
Copy link
Contributor

metrics of varying kinds (plugin run times, cache metrics, ...)

Currently we have a getTimings method provided on the bundle result.

Not at all debating - just trying to think about what info might be provided so that we can make sure the convention can scale well.

@shellscape
Copy link
Contributor Author

No worries. We can let this marinate for a bit too.

@lukastaegert
Copy link
Member

lukastaegert commented Aug 12, 2018

Very nice, this could really help in migrating plugins to new APIs by allowing them to support an old API in parallel. Some thoughts and questions:

  • How do we want to decide in the future if stuff is put onto meta or directly onto the plugin context? For now, one possible distinction could be that meta is for (more or less static) data while the context is for API functions?
  • In this spirit as a hypothetical example, one way to make timings available to plugins here could be: If the perf option is provided, meta contains an additional property that contains the currently collected timing information and which is mutated each time new timings are added.
  • How do we want to handle documentation? This should be documented soon, otherwise we might forget about it. This means we should be sure enough about this because people will start using it. For reference, this is where this information is collected at the moment: https://github.com/rollup/rollup/wiki/Plugins#plugin-context

@tivac
Copy link
Contributor

tivac commented Aug 12, 2018

I really hate that the plugin API lives in the wiki, because I always forget that the wiki even exists now that it's been 99% deprecated.

@shellscape
Copy link
Contributor Author

@tivac I'm absolutely planning to merge that to the main docs repo this week.

@lukastaegert I agree that it should only contain static data. That's always my intention when I use that moniker. And the ideas around perf data seem reasonable to me. I'll also update the wiki before this is merged. (and following, will be a PR on the docs site to move the plugin info)

Can we get a second approval to knock this one home?

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.

Can we get a second approval to knock this one home?

You totally got my approval, just wanted to clear this up before merging this. That being said, please merge at your own discretion 😉

@shellscape
Copy link
Contributor Author

Thanks @lukastaegert I'll work on the documentation later tonight or tomorrow. Keep an eye out for that one.

@guybedford guybedford merged commit 66b1439 into master Aug 24, 2018
@lukastaegert lukastaegert deleted the feat/options-version branch August 24, 2018 20:50
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.

Plugins need access to rollup version and/or to plugin API version
4 participants