Closed samlfair closed 1 year ago
The changes look good! I left a few suggestions re: types in the thread above.
I noticed there's a src/lib/slices/
directory. Are those Slice components only for the demo app? If so, could we move it out of src/lib/
to ensure they do not get published? Since the components are only used for the root route, you could move them adjacent to +page.svelte
(see the Svelte docs on colocation).
CI is failing due to an installation error. You may need to rebuild your package-lock.json
file. You can do so by deleting package-lock.json
and running npm install
. Normally, this would be a bad idea, but since you just updated all dependencies, it shouldn't cause any negative effects.
After those last changes, the PR looks good to merge into master
and into the individual component PRs. 🙂
I noticed there's a src/lib/slices/ directory. Are those Slice components only for the demo app? If so, could we move it out of src/lib/ to ensure they do not get published?
Yep, that makes sense. This highlights a weirdness with Svelte packaging: components would normally go in your lib
directory, so putting them in the routes
directory feels a little odd. However, when you're making a component library, you would really expect to only put components in the lib
directory, and this makes the codebase easier to understand. So, I agree that slices
should be in routes
.
CI is failing due to an installation error. You may need to rebuild your package-lock.json file. You can do so by deleting package-lock.json and running npm install.
Done. Hopefully that takes care of it.
I noticed there's a src/lib/slices/ directory. Are those Slice components only for the demo app? If so, could we move it out of src/lib/ to ensure they do not get published?
Yep, that makes sense. This highlights a weirdness with Svelte packaging: components would normally go in your
lib
directory, so putting them in theroutes
directory feels a little odd. However, when you're making a component library, you would really expect to only put components in thelib
directory, and this makes the codebase easier to understand. So, I agree thatslices
should be inroutes
.
Yeah, I can see how that would be confusing. If it makes more sense to keep the components in lib
, you could keep them there and add it to .npmignore
. Files and directories listed in .npmignore
do not get published to npm.
However, I would keep it how you have it now, or maybe even a step further by moving CodeSnippet.svelte
and RichText.svelte
directly adjacent to +page
so you don't have the slices
directory.
Keeping it out of lib
prevents future developers from thinking slices
are part of the published package.
There's one last change needed to clean up the types: CodeSnippetSlice.type.ts
can be converted to a type
using prismicT.Slice
's type parameters:
// src/types/CodeSnippetSlice.type.ts
import type * as prismicT from "@prismicio/types";
export type CodeSnippetSlice = prismicT.Slice<{
content: prismicT.RichTextField;
}>
After that change, everything looks good to merge! 🎉 Nice work!
Types of changes
Description
Upgrade SvelteKit to version 1 and upgrade TypeScript from 4.6 to 4.9.
Checklist: