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
Use vue cli 3 to set up the various webpack configurations #127
Conversation
@elevatebart I’ll try to find time to look into this later this week! Been away from computer for a week or so and haven’t had a chance yet to take a closer look. |
Quick thought: This already touches 34 files and has quite some tasks still open. Could it be easier (also to review) to do these step by step? The tasks seem rather unrelated to me, so maybe do it step by step? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments regarding your initial changes @elevatebart
.travis.yml
Outdated
- npm run build:app | ||
- yarn build:docs | ||
- yarn build:system | ||
- yarn build:app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason again for switching over to yarn by default instead of npm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the cli-plugins must have a specific format for their name I was thinking of keeping vue-design-system a thing and create another package that would be named vue-cli-plugin-vueds
.
Since it is cumbersome to have to maintain two repositories separately, mainly for testing, I was thinking that we could use yarn workspaces along with lerna to keep it all in here.
It works pretty well for me on vue-cli-plugin-styleguidist and very well for Evan You on @vue/cli.
Do you understand where I am going with yarn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elevatebart Makes sense to me!
resources: [ | ||
path.join(__dirname, "../src/assets/tokens/tokens.scss"), | ||
path.join(__dirname, "../src/assets/tokens/tokens.map.scss"), | ||
path.join(__dirname, "../src/styles/styles.scss"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we going to make tokens and SCSS helpers available globally for all components (previously here in webpackConfig)?
package.json
Outdated
"theo:onchange": "onchange \"./src/tokens/*.yml\" -- npm run theo", | ||
"test": "npm-run-all theo unit", | ||
"theo:onchange": "onchange \"./src/tokens/*.yml\" -- yarn theo", | ||
"test": "yarn theo && yarn unit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think && will work in windows which was the original reason for switching to npm-run-all. I think we should still support both windows & macOS in addition to Linux users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried it on windows and I believe it works. We can try it together if you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends how the user has set up their system. I’m 99% sure it won’t work for some users as there even was previously a bug about this in vueds that I had to fix (by getting rid of &’s). It’s also mentioned in multiple other places too like f.ex. npm-run-all’s readme.
package.json
Outdated
"styleguide:build": "vue-styleguidist build --config ./config/docs.config.js", | ||
"build:app": "yarn build", | ||
"build:system": "yarn build --target lib ./src/system.js", | ||
"build:docs": "yarn theo && yarn styleguide:build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will probably also throw an error for some Windows users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would that be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elevatebart because of & again :)
data: ` | ||
@import "@/assets/tokens/tokens.scss"; | ||
@import "@/assets/tokens/tokens.map.scss"; | ||
@import "@/styles/styles.scss"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where tokens and helpers get loaded globally? If yes, ignore my other comment regarding webpackConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is "documented" here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More specifically here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found it earlier too, thanks! :)
// In general you probably want this to be “umd”, | ||
// but for SSR/Nuxt.js you might want to use “commonjs2” | ||
// to avoid usage of window object. | ||
libraryTarget: "umd", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts, where would we be setting what options are used when a library build is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When typing vue-cli-service --target lib, a umd library is created. Options are limited but all documented on the vue cli website
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that might be great if we don't have to manage that part at all. As long as things work.
"./src/assets/tokens/tokens.scss", | ||
"./src/styles/_spacing.scss", | ||
"./src/styles/_mixins.scss", | ||
"./src/styles/_functions.scss", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on preserving some of the build configuration somewhere else? Will that be possible? Don’t think all of this can be just removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could set customization entry points for the build process in multiple places.
We could have a vueds.config.js or we could include it as pluginSettings directly in vue.config.js.
I would recommend though not to multiply the customization entry points. Every customization is source of bugs and misunderstandings. It can make the developers codebase very ugly and vue-design-systems changes incredibly more dangerous. Plus it is more documentation work ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree.
@elevatebart do you have thoughts regarding @HerrBertling’s comment? Personally don’t see it as a problem if this is one separate big branch as it might be quite hard to do these things in isolation from each other. Also don’t think any of these things could be merged in separately…? |
@viljamis, @HerrBertling I think it might be a grand idea, I just need to revert the switch to yarn since it is not necessary for this change. ...Unless of course if @viljamis you are confortable with switching to yarn. |
Hi @viljamis, I removed entirely the references to yarn from this branch. Yours, bart |
@elevatebart I will review thoroughly the new set of changes soon! |
b9e555a
to
723551e
Compare
723551e
to
589dd0a
Compare
8ae3402
to
59b4b75
Compare
Any news on this? I guess this could also ease the integration with typescript |
Feature branch for #83