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

refactor: vite for kv-components #507

Merged
merged 19 commits into from
Jan 17, 2025
Merged

refactor: vite for kv-components #507

merged 19 commits into from
Jan 17, 2025

Conversation

emuvente
Copy link
Collaborator

@emuvente emuvente commented Jan 10, 2025

Started doing this so that KvFlag and the flag svgs could be transpiled in the library, avoiding the need for UI, CPS, or other apps to handle it.

Most notable changes are

  • upgrading from node 18 to node 22
  • updating entry points for importing components. One can now either import { KvMap } from '@kiva/kv-components'; or import KvMap from '@kiva/kv-components/vue/KvMap'; or import KvMap from '@kiva/kv-components/vue/KvMap.js';
  • moving the components, stories, and utilities to the src directory
  • replacing dependencies with bundledDependencies and peerDependencies
  • kv-tokens is now 100% esm for source files
  • KvFlag now requires the country name to be passed in
  • KvCountdownTimer now takes a deadline prop for the Date to count down to instead of a msLeft prop for the number of milliseconds remaining

@mcstover
Copy link
Collaborator

What creates the need to have a folder and additional index for every component? Is there a way around that?

I can see the usefulness of synchronizing within this repo with or without the .vue extension but this still leaves a .vue usage for each component import in the index files. Could we just skip having to change that everywhere and allow .vue extensions in vite and eslint.

We could also look at using an alias in vite to resolve without a relative path...something like #components/MyComponent instead of ../../... It would of course also require a bunch file updates to implement... 🤷

@emuvente
Copy link
Collaborator Author

What creates the need to have a folder and additional index for every component? Is there a way around that?

This structure is from a guide for component libraries I was following. Each index is an entry point for the library. It allows individual fully-built components to be used without depending on the rest of the library and without needing an additional build step. We can also use those individual index files to inject type definitions, if/when we add typescript to this. If we use just the top-level index file, we might not get tree-shaking with the output and it might be harder to add type definitions. I'm still testing the consumer side of this setup, and I'll change it if I can.

We could also look at using an alias in vite to resolve without a relative path...something like #components/MyComponent instead of ../../... It would of course also require a bunch file updates to implement... 🤷

Yeah, that's a good idea. I probably should have started with that. Find & replace will make easy work of it at least

@emuvente emuvente marked this pull request as ready for review January 17, 2025 01:05
Copy link
Collaborator

@mcstover mcstover left a comment

Choose a reason for hiding this comment

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

This is great! And relatively few actual code changes!

…e storybook

BREAKING CHANGE: dist folder structure is different, so existing import statements will not work
…A-137

BREAKING CHANGE: country name must be provided to KvFlag
BREAKING CHANGE: components exported at vue/* to match source path
BREAKING CHANGE: modules now use import/export and .js extensions
…adline Date prop

BREAKING CHANGE: msLeft Number prop replaced with deadline Date prop
@emuvente emuvente merged commit c45dbb0 into main Jan 17, 2025
3 of 5 checks passed
@emuvente emuvente deleted the kv-components-vite branch January 17, 2025 20:00
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