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

build: migrate to vite #61

Merged
merged 54 commits into from
Feb 14, 2023
Merged

build: migrate to vite #61

merged 54 commits into from
Feb 14, 2023

Conversation

pirhoo
Copy link
Member

@pirhoo pirhoo commented Jan 31, 2023

Big PR to migrate to Vite, since it's the recommended way by the Vue CLI team.

Summary

The migration was done following those steps:

  • scaffolded vite.config.js based on @vue/create-vue
  • installation of @vitejs/plugin-vue2 to support Vue 2
  • added module import aliases, including @lib to refer to the root of the ./lib folder
  • rewrote of all the imports in components to match the new aliases
  • rewrote of the custom Webpack loaders to Vite plugins, in ./plugins
  • moved of the markdown-it plugins, in ./plugins/markdown-it
  • rewrote of the dynamic docs routes with a virtual plugin in ./plugins/docs.ts
  • fixed the docs' markdown files which are not always interpreted correctly by @vue/compiler-sfc
  • converted all docs' files still in .vue to .md to avoid maintaining both
  • migrated from jest to Vite
  • configured build of lib and doc in two separated config files

Observations

Documentation

To migrate the documentation to vite, we had to rewrite Webpack loaders into Vite plugins. This task was straight forward, with a few observations:

  • Markdown files using Vue components need to wrap those components between <template> tags
  • Markdown files using slotted scopes are sometimes not working. I couldn't identify why.
  • The list of pages used to be build using dynamic imports and loader. This is done now using a virtual plugin that returns all the pages metadata.

Tests

Jest is not officially supported by Vite. Vite team recommend to use Vitest which has a syntax very similar to Jest. Since using Jest with Vite requires a lot of extra work and is not straight-forward, we decided to take this opportunity to migrate to Vitest. The operation was seamless.

Assets

As of today, it is not officially supported to inline assets out of the bundle when using lib mode with Vite. This has consequences for components like TexturedDeck that load asset dynamically. To handle this we parameterized the host of some assets (all configurable in the Murmur's config):

@pirhoo pirhoo requested a review from a team January 31, 2023 14:23
@pirhoo pirhoo force-pushed the build/migrate-to-vite branch from ec568e9 to 6082569 Compare February 13, 2023 14:32
@pirhoo pirhoo marked this pull request as ready for review February 13, 2023 15:51
<button class="btn btn-info font-weight-bold" @click="$refs.formModal.show()">
Click to see the form
</button>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add a surrounding template tag like in other example?

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't understand why, but sometimes it's needed, and sometime it's not 🤷‍♂️

pirhoo and others added 2 commits February 14, 2023 18:23
@@ -32,11 +32,11 @@ import uniqueId from 'lodash/uniqueId'
import { faTimes } from '@fortawesome/free-solid-svg-icons/faTimes'
import { BTooltip } from 'bootstrap-vue/esm/components/tooltip/tooltip'

import { library } from './Fa'
import { library, default as Fa } from './Fa'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { library, default as Fa } from './Fa'
import Fa from './Fa'

To avoid importing twice the library object, maybe it could be interesting to just use Fa and then line 159 useFa.library (

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I know, we cannot use Fa.library

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so all of the other comments are pointless ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

😵

Copy link
Contributor

@caro3801 caro3801 left a comment

Choose a reason for hiding this comment

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

Impressive work, very interesting to walk through this PR. Well done 👍

lib/components/HapticCopy.vue Show resolved Hide resolved
lib/components/ImddbHeader.vue Show resolved Hide resolved
lib/components/SharingOptions.vue Show resolved Hide resolved
tests/unit/components/Fa.spec.js Show resolved Hide resolved
@@ -1,5 +1,5 @@
import { mount } from '@vue/test-utils'
import HapticCopy from '@/components/HapticCopy.vue'
import HapticCopy from '@lib/components/HapticCopy.vue'
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere (sorry it's vague) I saw this component imported with relative path (looked like something like that: import HapticCopy from './HapticCopy.vue' )

It could be great to change to look like this import HapticCopy from '@lib/components/HapticCopy.vue'

Copy link
Member Author

Choose a reason for hiding this comment

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

The imports are not consistent indeed. We should do a refactoring branch for that purpose. Maybe with additional alias for keys directories (@components, @maps, etc)?

@@ -176,7 +176,7 @@ describe('SharingOptionsLink', () => {
// Close the popup
$popup.instance.closed = true
// Wait for the interval to be called
jest.advanceTimersByTime(1000)
vi.advanceTimersByTime(1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

good to see that vitest supports this kind of feature

Copy link
Member Author

Choose a reason for hiding this comment

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

Vitest was made so "migrating from Jest is straightforward" and I can testify it is true!

@pirhoo
Copy link
Member Author

pirhoo commented Feb 14, 2023

Thanks @caro3801 for the review!

@pirhoo pirhoo merged commit 024890a into master Feb 14, 2023
@caro3801 caro3801 deleted the build/migrate-to-vite branch April 3, 2023 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants