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

APP-2072 Migrate PRIME to Svelte 4. #265

Merged
merged 3 commits into from
Jul 11, 2023
Merged

APP-2072 Migrate PRIME to Svelte 4. #265

merged 3 commits into from
Jul 11, 2023

Conversation

ethanlook
Copy link
Contributor

@mcous thanks for the inspiration for PR descriptions :)

Overview

This migrates PRIME to Svelte 4. I wanted to do this migration before changing the package structure.

Change log

  • npx svelte-migrate@latest svelte-4
  • Fix package version mismatches
  • Import styles directly instead of addStyles helper
  • Fix button and input attachInternals
  • Fix code editor injected styles
  • Change slider range prop to only accept a string - mixed types doesn't play well with web components
  • Use Svelte createEventDispatcher directly - get_current_component no longer works
  • Fix problematic tests

Review requests

Pull down the branch and make sure:

  • pnpm run test passes
  • Elements look/behave correctly with pnpm run storybook and pnpm start

Questions:

  • Is the minor version bump to 0.3.0 appropriate? I imagine we'll bump to 1.0.0 with the package restructure since that will be breaking.

@mcous
Copy link
Member

mcous commented Jul 6, 2023

thanks for the inspiration for PR descriptions :)

The template is here for the taking, if we want it!
https://github.com/Opentrons/opentrons/blob/edge/.github/PULL_REQUEST_TEMPLATE.md

@DTCurrie
Copy link
Member

DTCurrie commented Jul 6, 2023

thanks for the inspiration for PR descriptions :)

The template is here for the taking, if we want it! https://github.com/Opentrons/opentrons/blob/edge/.github/PULL_REQUEST_TEMPLATE.md

If there is no opposition to it I don't mind setting it as our template.

micheal-parks
micheal-parks approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@micheal-parks micheal-parks left a comment

Choose a reason for hiding this comment

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

🚀

@ethanlook
Copy link
Contributor Author

Bundle size is down with Svelte 4!

Before:

dist/prime.css   25.84 kB │ gzip:  5.49 kB
dist/prime.js   329.75 kB │ gzip: 72.66 kB
dist/prime.umd.cjs  210.17 kB │ gzip: 59.10 kB

After:

dist/prime.css   25.84 kB │ gzip:  5.49 kB
dist/prime.js   318.95 kB │ gzip: 71.46 kB
dist/prime.umd.cjs  190.12 kB │ gzip: 56.52 kB

mcous
mcous approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Tests and playground LGTM!

Is the minor version bump to 0.3.0 appropriate? I imagine we'll bump to 1.0.0 with the package restructure since that will be breaking.

Works for me. Since we're pre 1.0, 0.3.0 is not semver compatible with ^0.2.0, which should cover our bases

.eslintrc.cjs Outdated
@@ -5,6 +5,7 @@ module.exports = {
env: {
browser: true,
},
parser: '@typescript-eslint/parser',
Copy link
Member

Choose a reason for hiding this comment

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

[Nit] I think this might be set by @viamrobotics/eslint-config, and may not be needed. However, it is not harmful to include

src/elements/badge.svelte Outdated
@@ -25,3 +22,7 @@ addStyles();
>
{label}
</small>

<style>
@import './../../prime.css';
Copy link
Member

Choose a reason for hiding this comment

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

[nit] does ./.. === ../?

Suggested change
@import './../../prime.css';
@import '../../prime.css';

src/elements/badge.svelte Outdated
@@ -25,3 +22,7 @@ addStyles();
>
{label}
</small>

<style>
Copy link
Member

Choose a reason for hiding this comment

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

Is this running through PostCSS? I thought this was necessary:

Suggested change
<style>
<style lang="postcss">

DTCurrie
DTCurrie approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@DTCurrie DTCurrie left a comment

Choose a reason for hiding this comment

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

✅

@ethanlook ethanlook force-pushed the APP-2072-prime-svelte-4 branch 2 times, most recently from 10534d8 to ac4db69 Compare July 7, 2023 18:35
micheal-parks
micheal-parks approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@micheal-parks micheal-parks left a comment

Choose a reason for hiding this comment

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

🎉 glad we arrived at this solution

mcous
mcous approved these changes Jul 11, 2023
View reviewed changes
Copy link
Member

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Looks great! One question about the need for dynamic imports in main, but that can be moved to the restructuring PR

src/main.ts
const main = async () => {
// These components are exported as pure Svelte components.
const componentModules = {
'v-badge': await import('./elements/badge.svelte'),
Copy link
Member

Choose a reason for hiding this comment

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

Can these become static imports since we removed the tag side effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's totally reasonable. We'll do that when we restructure the package.

@ethanlook ethanlook merged commit 3c9d10b into viamrobotics:main Jul 11, 2023
5 checks passed
@ethanlook ethanlook deleted the APP-2072-prime-svelte-4 branch July 11, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcous mcous mcous approved these changes

@micheal-parks micheal-parks micheal-parks approved these changes

@DTCurrie DTCurrie Awaiting requested review from DTCurrie

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

Successfully merging this pull request may close these issues.
4 participants
@ethanlook @mcous @DTCurrie @micheal-parks