Skip to content
This repository has been archived by the owner on Apr 5, 2021. It is now read-only.

Remove unused dependencies #302

Merged
merged 3 commits into from
May 24, 2020
Merged

Remove unused dependencies #302

merged 3 commits into from
May 24, 2020

Conversation

SamChou19815
Copy link
Contributor

Summary

A lot of dependencies are unused, they seem to be a left over of the old Vue CLI

  • postcss related: Vue CLI enables them by default
  • corejs: both gridsome and vue already declares them as dependencies
  • eslint-import-resolver-webpack, eslint-config-airbnb-base: unused in the eslint config
  • some babel stuff related to typescript support for vue: it should't be difficult to support ts in vue any more. New vue cli has good support for that. I was able to enable ts for course plan without much changes: https://github.com/cornell-dti/course-plan/pull/56/files
  • eslint-plugin-import: already a dependency of vue's eslint plugins

Test Plan

  • Linter can run.
  • Website can be served locally.
  • Website can be built.

@SamChou19815
Remove unused dependencies
4231552 
### Summary

A lot of dependencies are unused, they seem to be a left over of the old Vue CLI

- postcss related: [Vue CLI enables them by default](https://cli.vuejs.org/guide/css.html#postcss)
- corejs: both gridsome and vue already declares them as dependencies
- eslint-import-resolver-webpack, eslint-config-airbnb-base: unused in the eslint config
- some babel stuff related to typescript support for vue: it should't be difficult to support ts in vue any more. New vue cli has good support for that. I was able to enable ts for course plan without much changes: https://github.com/cornell-dti/course-plan/pull/56/files
- eslint-plugin-import: already a dependency of vue's eslint plugins

### Test Plan

- Linter can run.
- Website can be served locally.
- Website can be built.
@dti-github-bot
Copy link
Member

dti-github-bot commented May 24, 2020 •
edited
Loading

[diff-counting] Significant lines: 33.

@ewlsh
Copy link
Member

ewlsh commented May 24, 2020 •
edited
Loading

You removed the site's support for tsx/jsx which vue-cli does not support well out-of-box currently last time I checked.

Additionally, core-js was explicitly defined because some libraries would pull in v2 instead otherwise. That may no longer be the case.

@ewlsh
Copy link
Member

ewlsh commented May 24, 2020

You also removed our SVG loader?

ewlsh
ewlsh suggested changes May 24, 2020
View reviewed changes
packages/website/package.json Show resolved Hide resolved
packages/website/package.json Show resolved Hide resolved
packages/website/package.json Show resolved Hide resolved
packages/website/package.json Show resolved Hide resolved
packages/website/package.json Show resolved Hide resolved
packages/website/package.json Show resolved Hide resolved
packages/website/package.json Show resolved Hide resolved
packages/website/vue.config.js Show resolved Hide resolved
@ewlsh
Copy link
Member

ewlsh commented May 24, 2020

I see CoursePlan's config did enable TSX support, I will have to look into this. When we upgraded TSX support was not functional out-of-box and required some tweaks and adjustments.

@SamChou19815 SamChou19815 requested a review from ewlsh May 24, 2020 22:01
@dti-github-bot
Copy link
Member

dti-github-bot commented May 24, 2020 •
edited
Loading

[deployment-bot] Deployed to https://5ecaf4c9d2193e07618e0411--cornelldti.netlify.app.

@SamChou19815
Copy link
Contributor Author

I see CoursePlan's config did enable TSX support, I will have to look into this. When we upgraded TSX support was not functional out-of-box and required some tweaks and adjustments.

All the svgs still loads, which shows that it is not needed.

@SamChou19815 SamChou19815 marked this pull request as ready for review May 24, 2020 22:03
@SamChou19815 SamChou19815 requested a review from a team as a code owner May 24, 2020 22:03
@ewlsh
Copy link
Member

ewlsh commented May 24, 2020 •
edited
Loading

I see CoursePlan's config did enable TSX support, I will have to look into this. When we upgraded TSX support was not functional out-of-box and required some tweaks and adjustments.

All the svgs still loads, which shows that it is not needed.

No they don't. You're looking in the wrong place... the vue.config.js does not build the site. It builds the CMS preview content which in this build no longer supports SVGs.

https://5ecaeec2c94566a8f55a1d28--cornelldti.netlify.app/admin/#/collections/pages/entries/page-sponsor

@ewlsh
Copy link
Member

ewlsh commented May 24, 2020 •
edited
Loading

We could unify our handling...

For the site we currently use: https://gridsome.org/plugins/gridsome-plugin-svg while for the CMS we use vue-svg-loader.

We could just use vue-svg-loader directly for the site too: https://gridsome.org/docs/assets-svg/#using-svgs-as-components

@ewlsh
Copy link
Member

ewlsh commented May 24, 2020 •
edited
Loading

We could unify our handling...

For the site we currently use: https://gridsome.org/plugins/gridsome-plugin-svg while for the CMS we use vue-svg-loader.

We could just use vue-svg-loader directly for the site too: https://gridsome.org/docs/assets-svg/#using-svgs-as-components

gridsome-plugin-svg is just a wrapper around vue-svg-loader

@ewlsh ewlsh merged commit 05e4be5 into master May 24, 2020
@ewlsh ewlsh deleted the remove-unused-deps branch May 24, 2020 22:35
@ewlsh
Copy link
Member

ewlsh commented May 24, 2020

@SamChou19815 changing the site's svg configuration can be punted to a separate PR if we choose to do it.

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

@ewlsh ewlsh ewlsh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SamChou19815 @dti-github-bot @ewlsh