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

Make router a separate plugin #4196

Merged
merged 7 commits into from Jul 5, 2019

Conversation

Projects
None yet
3 participants
@pksunkara
Copy link
Collaborator

commented Jun 25, 2019

Part of #2335. We remove the router option from root options and use it as a plugin as you can see here. This is the actual App.vue template for router which now makes it more consistent over different invoking methods.

id: `core:${name}`,
apply: loadModule(`@vue/cli-service/generator/${name}`, context)
})
if (id === 'router') {

This comment has been minimized.

Copy link
@pksunkara

pksunkara Jun 25, 2019

Author Collaborator

This if condition will exist until I make the cli-plugin-vuex PR.

@pksunkara

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

@LinusBorg @sodatea This is ready for review and merge.

@@ -112,15 +105,15 @@ vue add vuex
``` json
{
"useConfigFiles": true,
"router": true,

This comment has been minimized.

Copy link
@sodatea

sodatea Jul 1, 2019

Member

Though we now have a standalone router plugin, the legacy preset format (router and routerHistoryMode option) can and should still be supported, so as to reduce the migration costs.

This comment has been minimized.

Copy link
@pksunkara

pksunkara Jul 2, 2019

Author Collaborator

I added support for legacy router option but have it removed from docs completely.

Show resolved Hide resolved packages/@vue/cli-plugin-router/README.md
Show resolved Hide resolved packages/@vue/cli/lib/util/inferRootOptions.js Outdated
@pksunkara

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

This is now ready for another review.

if (cliOptions.bare) {
preset.plugins['@vue/cli-service'].bare = true
}

// legacy support for router
if (preset.router) {
preset.plugins['@vue/cli-plugin-router'] = {}

This comment has been minimized.

Copy link
@sodatea

sodatea Jul 4, 2019

Member

Need to support routerHistoryMode here.

@@ -10,8 +10,6 @@ const rcPath = exports.rcPath = getRcPath('.vuerc')
const presetSchema = createSchema(joi => joi.object().keys({
bare: joi.boolean(),
useConfigFiles: joi.boolean(),
router: joi.boolean(),

This comment has been minimized.

Copy link
@sodatea

sodatea Jul 4, 2019

Member

Though not preferred, they should remain in the schema, otherwise the legacy preset format cannot pass the schema validation.

Note: you may leave a TODO: comment here, once @hapi/joi v16 released, we can update the schema to utilizing its .warn feature. hapijs/joi@59e60aa

@sodatea

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

As I tested out, the router plugin does not work well with @vue/cli-service 3.x (maybe due to the REPLACE part?).
This may be an issue for users who have global @vue/cli v4.x installed and try to call vue add router on a v3 project. We can add a preflight check in generator/index.js to prompt users the incompatibility.

@pksunkara

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2019

As I tested out, the router plugin does not work well with @vue/cli-service 3.x (maybe due to the REPLACE part?).

I wanted to add the assert version stuff we recently added to vue but kinda forgot about it. It fixes this issue.

@pksunkara

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2019

Addressed your comments. This is now ready for another review.

@@ -3,9 +3,6 @@ const { toShortPluginId } = require('@vue/cli-shared-utils')

exports.getFeatures = (preset) => {
const features = []
if (preset.router) {

This comment has been minimized.

Copy link
@sodatea

sodatea Jul 5, 2019

Member

Don't delete this line. It's for displaying preset features.
image

Show resolved Hide resolved packages/@vue/cli-plugin-router/generator/template/src/App.vue Outdated
@sodatea

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Only 2 small modifications suggested.
The rest of this PR looks great to me.
Thanks for your efforts! 😄

@pksunkara

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2019

Updated

@sodatea

sodatea approved these changes Jul 5, 2019

@sodatea sodatea merged commit 246ae67 into vuejs:dev Jul 5, 2019

6 of 7 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci: cli-ui Your tests passed on CircleCI!
Details
ci/circleci: group-1 Your tests passed on CircleCI!
Details
ci/circleci: group-2 Your tests passed on CircleCI!
Details
ci/circleci: group-3 Your tests passed on CircleCI!
Details
ci/circleci: group-4 Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details

@pksunkara pksunkara deleted the pksunkara:router branch Jul 5, 2019

@vue-bot

This comment has been minimized.

Copy link

commented Jul 5, 2019

Hey @pksunkara, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.