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.

Sign up for GitHub

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

Jump to bottom

[Svelte] Svelte v4 support #1018

Merged
merged 1 commit into from
Nov 7, 2023
Merged

[Svelte] Svelte v4 support #1018

merged 1 commit into from
Nov 7, 2023

Conversation

ChqThomas
Copy link
Contributor

Q A
Bug fix? no
New feature? no
Tickets Fix #1012
License MIT

This PR updates the peerDependencies of ux-svelte to support Svelte 4

This includes a slight changes with the typescript types : https://svelte.dev/docs/v4-migration-guide#sveltecomponenttyped-is-deprecated

There are no BC breaks, Svelte 3 is still supported :

"peerDependencies": {
    "svelte": "^3.0 || ^4.0"
}

weaverryan
weaverryan reviewed Aug 7, 2023
View reviewed changes
src/Svelte/assets/package.json
@@ -21,7 +21,7 @@
},
"peerDependencies": {
"@hotwired/stimulus": "^3.0.0",
"svelte": "^3.0"
"svelte": "^3.0 || ^4.0"
},
"devDependencies": {
"@hotwired/stimulus": "^3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the devDependencies svelte also below? That will, in effect, mean that we'll be testing now with svelte 4... but our CI isn't rich enough (and maybe shouldn't be, as the matrix could get huge) to test multiple versions of the JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay!

I can't get the test suite to work with svelte 4 :(
Svelte 4 requires ESM and that would require running jest with the --experimental-vm-modules options.
I followed the guide from svelte-jester and tried a lot of options but can't find a working transformer config for our monorepo + ts setup

Copy link

Choose a reason for hiding this comment

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

Any update on this ? I would very much like to use Svelte but not being in v4 is a bit of a blocker.
Any chance to try to use vitest as the testing lib only for the Svelte package ?
The majority of svelte devs use Vite as it is part of the Svelte docs, so I don't think sticking with jest is a good choice for the maintainability

Copy link
Member

@weaverryan weaverryan Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This might be the same problem we're facing with chart.js v4 (it works fine, but we can't get the tests to pass because jest doesn't like esm). #610 (comment)

I'd love if someone experimented with switching to vitest and opened a PR - at least just to get things rolling and learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I quickly tried to install Vitest and just by following a migration guide 75% of the test suite pass
Maybe it will take a lot of work for the remaining 25% (some packages use specific jest mock libraries) but it may be possible!

Copy link
Member

Choose a reason for hiding this comment

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

That's great! Would you mind pushing your work on a separate PR? Then we could take a look and see what the remaining 25% is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weaverryan Here you go #1202 😉

@ChqThomas ChqThomas mentioned this pull request Oct 18, 2023
Merged
33 tasks
weaverryan added a commit that referenced this pull request Oct 22, 2023
@weaverryan
feature #1202 Migrate from Jest to Vitest (ChqThomas, weaverryan)
77b916a 
This PR was merged into the 2.x branch.

Discussion
----------

Migrate from Jest to Vitest

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Tickets       | #1018 #610
| License       | MIT

This PR is a draft to test the feasibility of migrating from [jest ](https://jestjs.io/) to [vitest](https://vitest.dev/)

Jest doesn't support ESM out of the box, which makes it difficult to test some libraries like Chart.js and Svelte v4. Vitest supports typescript and ESM by default and is compatible with Jest API, it'll help us test those packages and maybe others in the future

#### Infos:

There is a global config file `vitest.config.js` which contains the default settings for every package, when needed it can be extended with a file in the package root (ex: `src\React\assets\vitest.config.js`)

I replaced the `yarn test` script with `vitest src --run` so it's run globally and the CI doesn't stop at the first error from a package, it still can be run for every/individual packages with `yarn workspaces run vitest --run` / `yarn workspace "`@symfony`/ux-live-component" run vitest --run`

Libraries like fetch-mock-jest and jest-canvas-mock need to be replaced
The `require_context_poylfill.ts` file used by ux-react, ux-vue and ux-svelte needs to be modified as it uses a dynamic `require`, replacing it with an `import` doesn't work as it is asynchronous

#### Resources:
- https://vitest.dev/guide/migration.html
- https://srivastavaankita080.medium.com/jest-vitest-migration-79f4735dd5d0

### TODO:
-   Packages:
    -   [x] `@symfony`/ux-autocomplete `0/14`
        -   [x] replace fetch-mock-jest ?
    -   [x] `@symfony`/ux-chartjs `0/4`
        -   [x] fix failing tests
        -   [x] replace jest-canvas-mock ?
    -   [x] `@symfony`/ux-cropperjs `1/1`
    -   [x] `@symfony`/ux-dropzone `3/3`
    -   [x] `@symfony`/ux-lazy-image `1/1`
    -   [x] `@symfony`/ux-live-component `217/217` 💯
        -   [x] `Error: Uncaught [ReferenceError: Node is not defined]` Just a warning but the test suite pass
    -   [x] `@symfony`/ux-notify `1/1`
    -   [x] `@symfony`/ux-react `0/4`
        -   [x] install `@vitejs`/plugin-react
        -   [x] migrate `require_context_poylfill.ts`
    -   [x] `@symfony`/stimulus-bundle `1/1`
        -   [x]  `Error: Uncaught [ReferenceError: Node is not defined]` Just a warning but the test suite pass
    -   [x] `@symfony`/ux-svelte `0/4`
        -   [x] replace svelte-jester with `@sveltejs`/vite-plugin-svelte
        -   [x] migrate `require_context_poylfill.ts`
        -   [x] fix failing tests
    -   [x] `@symfony`/ux-swup `0/5`
        -   [x] `require() of ES Module node_modules/delegate-it/index.js from node_modules/swup/lib/index.js not supported.`
    -   [x] `@symfony`/ux-toggle-password `1/1`
    -   [x] `@symfony`/ux-translator `91/91`
    -   [x] `@symfony`/ux-turbo `0/2`
        -   [x] `TypeError: Cannot read properties of undefined (reading 'prototype')`
    -   [x] `@symfony`/ux-typed `1/1`
    -   [x] `@symfony`/ux-vue `0/5`
        -   [x] replace `@vue`/vue3-jest with `@vitejs`/plugin-vue
        -   [x] migrate `require_context_poylfill.ts`
-  [x] Remove jest
-  [x] Cleanup
-  [ ] Benchmark ?

I'll need help to resolve these issues 😁

Commits
-------

76d82de Removing need for jq
7f37e59 vitest.config.js format
f953d52 Remove jest config and dependencies
de32106 Import vitest when needed instead of adding global types in tsconfig
9322bc7 format
2be4088 re-adding module
a33fc1d Fixing final tests
f59f4f6 Working around Turbo issue
13c7431 dropping support for very old versions of swup
ecba87b Adding script to run all tests, but keep going if a previous one fails
238d25e Stopping Stimulus to avoid side effects after the test
cf29587 Replacing require.context polyfill with a simpler implementation
ef34031 Swapping in vitest-canvas-mock
2270406 Switching to vitest-fetch-mock
9043da3 Removing no-longer-needed setup file
c1db340 Fixing problem where Stimulus sometimes continued after the test
10c5591 Add forgotten package.json from ux-react package
e9e16eb Wip trying to migrate from jest to vitest
@ChqThomas
Copy link
Contributor Author

Thanks to Vitest it's now ready!

weaverryan
weaverryan reviewed Oct 22, 2023
View reviewed changes
src/Svelte/doc/index.rst

# or use yarn
$ yarn add svelte svelte-loader --dev
$ yarn add svelte-loader --dev
Copy link
Member

Choose a reason for hiding this comment

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

svelte itself is no longer needed in the user's project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is automatically added when you install symfony/ux-svelte, I think I removed it to be consistent with the other docs

@weaverryan
Copy link
Member

Thank you for pushing on this Thomas!

@weaverryan weaverryan merged commit 3a127fe into symfony:2.x Nov 7, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aqmebrak aqmebrak aqmebrak left review comments

@weaverryan weaverryan weaverryan left review comments

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

Successfully merging this pull request may close these issues.

[Svelte] Warning / suggested configuration when installing via composer
3 participants
@ChqThomas @weaverryan @aqmebrak