Skip to content

Instantly share code, notes, and snippets.

@sodatea
Last active June 26, 2019 17:32
Show Gist options
  • Save sodatea/37bc8c36d7d0c285dfea52fbe8ddaf90 to your computer and use it in GitHub Desktop.
Save sodatea/37bc8c36d7d0c285dfea52fbe8ddaf90 to your computer and use it in GitHub Desktop.

v4

Planned Breaking Changes

  • ESLint has released v6, should upgrade, contribution welcome
  • vue upgrade, almost done, needs tests, assigned to myself, expected to get it done before Thursday. After that we should start writing migration documentation.
  • Make vuex & vue-router standalone plugins vuejs/vue-cli#4196

Needs to Fix

Planned Non-Breaking Changes

  • Plugin Ordering

    After thinking about this, I realized it's not that urgent than I previously thought. There're all kinds of workarounds for the use cases (either by hacking the API or by changing the plugin UX flow). I prefer not to touch the code base at the moment in favor of stability. We don't have that much time and energy to fix all the possible bugs coming with this big feature.

    We can add this later in a feature release if we come to a consensus on the design and implementation.

  • Warn before Generator is invoked and might override uncommited changes This is needed for vue upgrade as we'll try to automatically migrate users' code

Other TODOs

See:

Some issues have the "good first issue" label added but I didn't check them all so maybe there are more easy ones. There're also other "nice to have" features with the "enhancement" label, I haven't looked into them thoroughly so if you are willing to pick one up, please comment at it first.

@pksunkara
Copy link

I would implement Plugin Ordering for v4 because it would be a breaking change. With the addition of assert*Version, it would be better to implement this now rather than releasing v4 without it.

@sodatea
Copy link
Author

sodatea commented Jun 25, 2019

I don't think it's that urgent.

In most cases when we need plugin ordering we use a preset. Though the plugins field in preset is an object, it actually has a guaranteed enumeration order in Node. (We can improve this by allowing arrays to be passed, that's not a breaking change though)

When a plugin is used independently, it can always check for package.json to infer existing plugins, and then prompt the user for possible actions to take next.

The currently proposed API is somehow too verbose IMHO. Maybe what users need is a way to delegate other plugin invocation rather than dealing with all kinds of generator lifecycle hooks.

@pksunkara
Copy link

Maybe what users need is a way to delegate other plugin invocation rather than dealing with all kinds of generator lifecycle hooks.

I like how you phrased it. This is what I want to implement before v4.

@sodatea
Copy link
Author

sodatea commented Jun 25, 2019

It'll be great if you have a detailed proposal. I'm also willing to add it in v4.0 if the outcome is not too complicated.

@pksunkara
Copy link

The Hook API proposal was meant to solve this. But I will try to make it simple and propose it again this week.

@pksunkara
Copy link

So, after some careful thinking, I agree with what you said above completely. In fact, I have already developed some plugins that do the exact same thing.

When a plugin is used independently, it can always check for package.json to infer existing plugins, and then prompt the user for possible actions to take next.

But I think a better way to solve this would be to add an assertPluginVersion('plugin-name', 'version') like how we did for the cli and service versions.

The currently proposed API is somehow too verbose IMHO. Maybe what users need is a way to delegate other plugin invocation rather than dealing with all kinds of generator lifecycle hooks.

Unfortunately, I don't think delegation is enough. It's good when you are depending on a plugin you control, but it's not enough when the plugin you want to delegate is not under your control. Also, what about the scenarios where you want all plugins to always delegate to your plugin for something but they don't? (Typescript plugin is the biggest example for this).

Coming to this, we already have 2 hooks in the current code which are just renamed in the proposal to make them more easier to understand what they do.

postProcessFiles -> afterRender
onCreateComplete -> afterWrite

We need afterAnyRender because I really don't imagine any other way of achieving the following situation:

Typescript plugin would want to convert all the files irrespective of which plugin they were generated from before writing them to the disk.

Sure, we can leave the typescript generation to individual plugins but they will not be under our control (they are 3rd party) and they might not implement it, which means more trouble for the end user.

Similarly, we need afterAnyWrite for the same thing but for eslint plugin.

And finally, the beforeAnyRender is needed so that we can decouple any conversion plugins. As a plugin writer, I don't want to think whether the user is using typescript or some other language and try to support all of them. This hook can be implemented by those converter plugins and make life easier for the plugin writers.

As you can see, the PR is just 100 lines to implement the above lifecycle hook system. But, I am always up for more discussion on this in case I am missing anything. Please do propose something if you have a counter proposal.

@sodatea
Copy link
Author

sodatea commented Jun 26, 2019

But I think a better way to solve this would be to add an assertPluginVersion('plugin-name', 'version') like how we did for the cli and service versions.

What about extending the hasPlugin helper to take a second argument that accepts a version range?

Typescript plugin would want to convert all the files irrespective of which plugin they were generated from before writing them to the disk.

Seems users want the other way around. vuejs/vue-cli#2676
If the plugin didn't implement typescript generation, we should ignore it.

Similarly, we need afterAnyWrite for the same thing but for eslint plugin.

We can use git commit hooks instead.

And finally, the beforeAnyRender is needed so that we can decouple any conversion plugins. As a plugin writer, I don't want to think whether the user is using typescript or some other language and try to support all of them. This hook can be implemented by those converter plugins and make life easier for the plugin writers.

I think this can be better done with helper functions like transformScript.

@pksunkara
Copy link

What about extending the hasPlugin helper to take a second argument that accepts a version range?

I think it's better if we provide an assert API to standardise the error messages along with the ones in vuejs/vue-cli#4000. We can improve the UI later to provide more info about the version mismatches like how vuejs/vue-cli#3926 did.

We can use git commit hooks instead.

I think this can be better done with helper functions like transformScript.

In that case, I am okay with the delegate idea. How about delegate('plugin', 'functionName', options)?

But this still doesn't solve the issue which was mentioned in the main comment of vuejs/vue-cli#1754.

I want to replace default import of PWA plugin (import './registerServiceWorker') to :

if (process.client) require('./registerServiceWorker');

ATM, I use api.postProcessFiles() function, but this one only work if I do:

vue add @vue/pwa
vue add my-plugin

But if I do

vue add my-plugin
vue add @vue/pwa

my-plugin is not aware that PWA was added/invoked, so I cant transform the import statement.

delegate doesn't help here because @vue/pwa is not something under the control of the plugin author there and he can't call delegate('my-plugin', 'replace') from inside it.

@sodatea
Copy link
Author

sodatea commented Jun 26, 2019

I think it's better if we provide an assert API to standardise the error messages along with the ones in vuejs/vue-cli#4000.

I think unlike PluginAPI and GeneratorAPI, plugin version requirements can be very loose. For example, most of our official plugins can support both @vue/cli-service v3 & v4, but may take different actions. So I prefer a less restrictive API.

How about delegate('plugin', 'functionName', options)?

Could you elaborate a little bit? I'm not sure what functionName is for.

But this still doesn't solve the issue which was mentioned in the main comment of vuejs/vue-cli#1754.

Yeah, that is a legit use case.
So we do need a hook after plugin invocation. This can also be used for later-added TS support.

@pksunkara
Copy link

I think unlike PluginAPI and GeneratorAPI, plugin version requirements can be very loose. For example, most of our official plugins can support both @vue/cli-service v3 & v4, but may take different actions. So I prefer a less restrictive API.

You are right. hasPlugin('plugin', 'version') is good then.

Could you elaborate a little bit? I'm not sure what functionName is for.

A plugin should be choosing what functions of it other plugins can call (Standard inversion control pattern). Eslint can have only one, lint. But typescript can have multiple convert, compile etc... That is what functionName is for.

So we do need a hook after plugin invocation

That was the reason I had afterAnyWrite in the first place.

@sodatea
Copy link
Author

sodatea commented Jun 26, 2019

So I'm convinced on this particular hook.
Now that other hooks are not needed, maybe we can choose a more intuitive name for it, such as afterGeneration.
And to keep the API interface minimal, we don't need a separate file or directory to store it. What about exposing it as a property of the generator function?

E.g.

// generator.js
module.exports = (api) => {
  // ...
}

module.exports.hooks = {
  'afterGeneration': (api) => {
    // ...
  }
}

@sodatea
Copy link
Author

sodatea commented Jun 26, 2019

As for the delegate API, I don't feel right but I'm not sure what exactly can be improved.
We can discuss it in a separate issue, though.

@pksunkara
Copy link

pksunkara commented Jun 26, 2019

maybe we can choose a more intuitive name for it, such as afterGeneration

How about afterAnyInvoke? We can move createComplete to afterInvoke. And leave postProcessFiles as it is.

module.exports.hooks

We don't need a separate object. api.afterAnyInvoke inside the generator should be enough. The reason I made a separate file is because if you see this, any plugin invocation will load all the other installed plugins to see if they have a hook. For performance reasons, instead of loading the whole generator module of the plugin, if they have a separate file called hooks.js, then it's easier to load only that one file. I am okay either way they are loaded.

delegate API

Yeah with afterAnyInvoke existing, I don't think delegate is urgent for now. We can talk about it as a separate issue.

@sodatea
Copy link
Author

sodatea commented Jun 26, 2019

afterAnyInvoke and afterInvoke looks good to me if they are both used inside the generator. But if that's the case, does it mean we have to actually run the generator to check hooks? Won't that be too much?

@pksunkara
Copy link

But if that's the case, does it mean we have to actually run the generator to check hooks? Won't that be too much?

That was the reason for a separate hook.js file.

@sodatea
Copy link
Author

sodatea commented Jun 26, 2019

Then I personally prefer the additional export object solution. Reasons:

  1. Implementation details aside, generator hooks are conceptually part of the generator utility itself.
  2. Usually, hooks share logic with the original generator, it makes more sense to keep them in one place.
  3. It has a smaller chance of breaking existing code so we can introduce it in v3 too.

Thoughts?

@pksunkara
Copy link

Sounds good to me. Should I update the PR with everything we finalized here?

@sodatea
Copy link
Author

sodatea commented Jun 26, 2019

Yeah go ahead please

@pksunkara
Copy link

Will do as soon as the current router and vuex packages are done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment