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(vue-app): add <client-only> alias for <no-ssr> #5941

Merged
merged 9 commits into from
Jul 4, 2019

Conversation

bluelovers
Copy link

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@codecov-io
Copy link

codecov-io commented Jun 16, 2019

Codecov Report

Merging #5941 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #5941   +/-   ##
=======================================
  Coverage   95.69%   95.69%           
=======================================
  Files          82       82           
  Lines        2693     2693           
  Branches      692      692           
=======================================
  Hits         2577     2577           
  Misses         98       98           
  Partials       18       18
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.27% <ø> (ø) ⬆️
#unit 92.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
packages/vue-app/src/index.js 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f3709c...b6521fe. Read the comment docs.

@atinux
Copy link
Member

atinux commented Jun 17, 2019

Actually I thought about the same 👍

@atinux atinux changed the title all alias name <ClientOnly> for <no-ssr> feat(vue-app): add <client-only> alias for <no-ssr> Jun 17, 2019
@atinux atinux requested a review from a team June 17, 2019 10:28
kevinmarrec
kevinmarrec previously approved these changes Jun 17, 2019
Copy link
Contributor

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

@pimlie
Copy link

pimlie commented Jun 17, 2019

Does this mean that we are going to deprecate <no-ssr>?

Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

How about adding alias in NoSsr and use Vue.component(NoSsr.alias, NoSsr) here

@kevinmarrec
Copy link
Contributor

@pimlie I don't think so, it's just an alias there won't be any depreciation
@clarkdo Nice one, we should do same for NuxtChild then :)

@pimlie
Copy link

pimlie commented Jun 17, 2019

I believe this is a solution to a problem that doesnt exists and therefore adds too much (useless) configuration. Although there might be reasons for changing this I am unaware of?

@manniL posted a nice link what mentions this yesterday on discord

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Jun 17, 2019

@pimlie I'd say the reason is just about DX.
It makes me think about something : Even if not used, aliases (which still remain new registered global Vue components) make JS bundle higher right ?

@pi0
Copy link
Member

pi0 commented Jun 17, 2019

Configurable alias makes integration harder and fragments users. Like a module that adds components that rely on no-ssr and we change the alias to something else in the project. I think we can rename no-ssr to client-only and deprecate no-ssr (for b-w compatibility) if it is better DX by using new name.

@clarkdo
Copy link
Member

clarkdo commented Jun 17, 2019

If replacing NoSsr with ClientOnly is the choice, I don't think it's a good idea to change it since NoSsr has been there for a long time and well documented. I think it's a understandable name as well, another reason is for keeping consistency with https://github.com/egoist/vue-no-ssr.

@bluelovers
Copy link
Author

bluelovers commented Jun 17, 2019

i think should keep no-ssr, just add a alias name

i didn't use Vue.component(NoSsr.alias, NoSsr), is because follow exists code style

why use ClientOnly for alias name, because we can keep some exists vue code to nuxt form vuepress

@manniL
Copy link
Member

manniL commented Jun 17, 2019

This could already be done in userland. If we adopt a new name, we should enforce it as well and deprecate no-ssr IMO, so we keep consistency.

@galvez
Copy link

galvez commented Jun 17, 2019

+1 on introducing alias-only.

@atinux
Copy link
Member

atinux commented Jun 19, 2019

Indeed VuePress has a <client-only> component that does the same as <no-ssr> (except they don't handle placeholder): https://vuepress.vuejs.org/guide/using-vue.html#browser-api-access-restrictions

Actually, for DX, client-only is better since there is no negation in the name.

I would suggest to rename to and set up an alias for <no-ssr>.

  • deprecate in favour of <client-only> (Nuxt 3)?

@atinux
Copy link
Member

atinux commented Jul 3, 2019

To make this PR valid, I recommend to:

@bluelovers
Copy link
Author

where is doc?

image

@atinux atinux mentioned this pull request Jul 4, 2019
@atinux
Copy link
Member

atinux commented Jul 4, 2019

I did the changes required + added a PR on the docs.

@atinux atinux requested review from clarkdo and a team July 4, 2019 14:14
@clarkdo
Copy link
Member

clarkdo commented Jul 4, 2019

Please open a pr for doc in https://github.com/nuxt/docs/blob/master/en/api/components-no-ssr.md

I'll do it.

@atinux
Copy link
Member

atinux commented Jul 4, 2019

@clarkdo already did: nuxt/docs#1472

@clarkdo clarkdo merged commit 9524bca into nuxt:dev Jul 4, 2019
@clarkdo
Copy link
Member

clarkdo commented Jul 4, 2019

@atinux PR for Chinese nuxt/docs#1473

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants