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

feat!: new architecture, update to Svelte 5 and refactor core lib #1127

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

edodusi
Copy link
Contributor

@edodusi edodusi commented Jan 14, 2025

Major Refactor: Svelte 5 Update & Architecture Improvements

This PR introduces significant improvements to the Storyblok Svelte library, modernizing its architecture and upgrading to Svelte 5.

  • Svelte 5 Migration: Updated core components to utilize Svelte 5's new features including:

    • Runes ($state, $props, $derived)
    • Modern component patterns
    • Enhanced TypeScript support
  • Architecture Refactoring:

    • Codebase aligned with Storyblok SDK ecosystem new standard
    • pnpm workspaces, vite, vitest, Cypress
    • unit and e2e tests added (initial round)
  • Core Library Improvements:

    • Implemented singleton store pattern for better state management
    • Improved component registration system
    • Better separation of concerns between modules
    • Modernized StoryblokComponent implementation

Please test it with the included playground or by integrating it in a custom project

Copy link
Collaborator

@alexjoverm alexjoverm left a comment

Choose a reason for hiding this comment

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

@edodusi Thanks a bunch for the set of improvements implemented here, and keeping this library to the latests standards. I've added a few comments, let me know if you need any clarification

update-types:
- patch
ignore:
- dependency-name: '*'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@edodusi wouldn't the ignore statement prevent the releases from working properly? Because it'll ignore also the updates coming from @storyblok/js.

If that's the case, can we set it to "ignore all dependencies except @storyblok/js"? cc @alvarosabu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexjoverm look at the line after, this is to tell dependabot to ignore all minor and major versions of all packages, and focus only on patch.

This is just for security updates, I should include a new rule for storyblok packages

Copy link
Collaborator

Choose a reason for hiding this comment

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

@edodusi in the way is set up, on the lines above (the groups statement) it where it groups the security updates. But the ignore applies to all minors and majors, not only security ones. Correct?

If that's the case, if in @storyblok/js we publish a new minor release, wouldn't that be ignored by this pkg? If the idea is to have a separate dependabot config for storyblok package, is it included in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes exactly, because the desired outcome would be that dependabot only opens auto-approved PR with grouped patches (we can decide to auto-merge them), and we control manually minors and majors because sometime there are changes that alter some stuff even in minor updates and it's better we do that manually

For storyblok packages we should have specific rules, because maybe we want patches and minors to be automatically merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

"because maybe we want patches and minors to be automatically merged" -> Exactly, we need it as that's how the whole release system works currently. Can we add them?

About security updates, makes total sense 👍

.github/workflows/dependabot-automerge.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot-automerge.yml Show resolved Hide resolved
package.json Show resolved Hide resolved
src/lib/storyblok.ts Outdated Show resolved Hide resolved
src/lib/storyblokStore.ts Outdated Show resolved Hide resolved
src/tests/storyblokStore.spec.ts Outdated Show resolved Hide resolved
src/app.d.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

@edodusi are the app.d.ts, app.html and routes/+page.svelte necessary as part of the library source code? Aren't they more of a playground files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is package structure as scaffolded by the Svelte CLI for a library

Copy link
Collaborator

Choose a reason for hiding this comment

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

What are they used for? It could happen the CLI creates them as a playground, but we have a separate one for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are not a playground but part of a library that we don't use, I can remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually app.html is required by svelte-package, so we need it

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is it needed for exactly? I still don't see the use as src of the library

Maybe is taken as the entry point for bundling?

src/routes/+page.svelte Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Blocking:

@edodusi Is it necessary to have an extra directory called lib? I would rather suggest having one for components like the @storyblok-vue one and the rest in the root of src.

Screenshot 2025-01-15 at 16 46 23

I see we have also a different structure on @storyblok-react. Would we nice if we standardized a little bit and avoided complex directory structures.
Screenshot 2025-01-15 at 16 47 27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alvarosabu this is the required structure for a Svelte library that builds with svelte-package, see https://svelte.dev/docs/kit/packaging

Running the svelte-package command from @sveltejs/package will take the contents of src/lib and generate a dist directory (which can be configured) containing the following:

All the files in src/lib. Svelte components will be preprocessed, TypeScript files will be transpiled to JavaScript.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. If it's possible to configure I will just set it to src but I respect svelte packages' best practices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be configured -i / --input — the input directory which contains all the files of the package. Defaults to src/lib but I would stick with Svelte best practice, what do you think

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal point of view, agree on embracing the Svelte best practice and defaults, where possible.

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.

4 participants