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

refactor: improve install function #3247

Merged
merged 7 commits into from Feb 20, 2018
Merged

refactor: improve install function #3247

merged 7 commits into from Feb 20, 2018

Conversation

KaelWD
Copy link
Member

@KaelWD KaelWD commented Feb 10, 2018

  • BREAKING CHANGE: must Vue.use(Vuetify) when using CDN package
  • You can now set the theme easily when using a CDN
  • No longer includes the entire package.json in the dist
  • Vue version is checked in the Vuetify component, now works with a-la-carte too
  • $vuetify is a proper vue instance instead of the hacky defineReactive
  • Vuetify.version is now available to CDN users
  • Replaced semver with a basic custom function, reduces gzipped size from 62KB to 56KB

How Has This Been Tested?

  • vuetify dev environment with Playground.vue
  • vuetifyjs.com repo
  • basic html file with markup from below

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

Checklist:

  • My change requires a change to the documentation.
  • I have created a PR in the documentation with the necessary changes.

Markup

<link rel="stylesheet" href="../dist/vuetify.min.css">
<script src="https://unpkg.com/vue"></script>
<script src="../dist/vuetify.min.js"></script>
<div id="app">
  <v-app>
    <v-content>
      <v-text-field v-model="primary" label="primary"></v-text-field>
      <v-btn color="primary">button</v-btn>
    </v-content>
  </v-app>
</div>
<script>
  Vue.use(Vuetify, {
    theme: { primary: '#BADA55' }
  })
  new Vue({
    el: '#app',
    computed: {
      primary: {
        get () { return this.$vuetify.theme.primary },
        set (val) { this.$vuetify.theme.primary = val }
      }
    }
  })
</script>

- BREAKING CHANGE: must Vue.use(Vuetify) when using CDN package
- No longer includes the entire package.json in the dist
- Vue version is checked in the Vuetify component, now works with
  a-la-carte too
- $vuetify is a proper vue instance instead of the hacky defineReactive
- Vuetify.version is now available to CDN users
@KaelWD KaelWD added the T: enhancement Functionality that enhances existing features label Feb 10, 2018
@KaelWD KaelWD added this to the Sprint 3 milestone Feb 10, 2018
@codecov
Copy link

codecov bot commented Feb 10, 2018

Codecov Report

Merging #3247 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3247      +/-   ##
==========================================
- Coverage   84.36%   84.35%   -0.01%     
==========================================
  Files         145      145              
  Lines        3370     3369       -1     
  Branches     1074     1075       +1     
==========================================
- Hits         2843     2842       -1     
  Misses        391      391              
  Partials      136      136
Impacted Files Coverage Δ
src/components/Vuetify/index.js 100% <100%> (ø) ⬆️

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 f0a0ca0...3627d86. Read the comment docs.

@KaelWD KaelWD changed the title enh: improve CDN usage refactor: improve install function Feb 10, 2018
nekosaur
nekosaur previously approved these changes Feb 12, 2018
theme: theme(opts.theme),
options: options(opts.options),
goTo
process.env.NODE_ENV !== 'test' && checkVueVersion(Vue)
Copy link
Member

Choose a reason for hiding this comment

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

Why shouldn't we check vue version in test env?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the tests don't go through webpack, so each test dumps "Vuetify requires Vue version undefined" to the console.

@johnleider johnleider removed this from the Sprint 3 milestone Feb 14, 2018
@KaelWD KaelWD changed the base branch from dev to master February 15, 2018 13:42
johnleider
johnleider previously approved these changes Feb 17, 2018
reduces gzipped package size by 6.2kb compared to master
@KaelWD KaelWD dismissed stale reviews from johnleider and nekosaur via 3627d86 February 17, 2018 17:12
@KaelWD KaelWD merged commit b45633c into master Feb 20, 2018
@KaelWD KaelWD deleted the fix/cdn-import branch February 20, 2018 12:53
@ekoeryanto
Copy link
Contributor

nuxt development broke when updating to 1.0.2

@ram-you
Copy link

ram-you commented Feb 20, 2018

@ekoeryanto I confirm the error with the message:
TypeError: Cannot read property 'split' of undefined

@beatgrabe
Copy link
Contributor

beatgrabe commented Feb 21, 2018

You can temporally resolve the issue by adding this to your webpack config:

const webpack = require('webpack')


module.exports = {
  
  plugins: [
    new webpack.DefinePlugin({
      'process.env.REQUIRED_VUE': '"<your vue version>"'
      // e.g. 'process.env.REQUIRED_VUE': '"2.5.13"'
    })
  ]
}

@ram-you
Copy link

ram-you commented Feb 21, 2018

Thank you @beatgrabe . Worked for me.

@JoseGoncalves
Copy link

With webpack and an a-la-carte setup the same error happens with 1.0.2: vuetifyjs/a-la-carte#6
While defining the REQUIRED_VUE environment variable would solve the issue, is this the recommended way to go?

@beatgrabe
Copy link
Contributor

The related error is now fixed in v1.0.3

@lock
Copy link

lock bot commented Apr 15, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please direct any non-bug questions to our Discord

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants