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

Types now working with SvelteKit 2 #176

Merged
merged 8 commits into from
Jan 2, 2024
Merged

Conversation

ryanylee
Copy link
Contributor

@ryanylee ryanylee commented Dec 14, 2023

Fixes #173

Copy link

@AdrianGonz97 AdrianGonz97 left a comment

Choose a reason for hiding this comment

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

It is also recommended for Svelte libraries to modify their moduleResolution and set it to NodeNext in their tsconfig.json, which adds stricter rules for import paths that enables wider compatibility for consumers.

package.json Outdated
},
"peerDependencies": {
"svelte": "^3 || ^4"
"svelte": "^4.2.8"

Choose a reason for hiding this comment

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

This is a breaking change and shouldn't be modified.

Suggested change
"svelte": "^4.2.8"
"svelte": "^3 || ^4"

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, we still want to support Svelte 3 as their APIs are similar.

},
"type": "module",
"svelte": "./index.js",

Choose a reason for hiding this comment

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

The svelte field should stay for backwards compatibility for older tooling. This would also be considered a breaking change. Updating it to point to the index.js in dist should suffice.

Choose a reason for hiding this comment

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

according to the vite docs, older tooling has always had support for this , and the use of
"svelte": "./index.js",
was just being supported since vite 3 , and support for that dropped in vite 5. so older tooling will work seamlessly with the use of exports

package.json Outdated
Comment on lines 83 to 94
"exports": {
".": {
"types": "./dist/lib/index.d.ts",
"svelte": "./dist/index.js",
"import": "./dist/index.js"
},
"./plugins": {
"types": "./dist/lib/plugins/index.d.ts",
"svelte": "./dist/plugins/index.js",
"import": "./dist/plugins/index.js"
}
},

Choose a reason for hiding this comment

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

Suggested change
"exports": {
".": {
"types": "./dist/lib/index.d.ts",
"svelte": "./dist/index.js",
"import": "./dist/index.js"
},
"./plugins": {
"types": "./dist/lib/plugins/index.d.ts",
"svelte": "./dist/plugins/index.js",
"import": "./dist/plugins/index.js"
}
},
"exports": {
".": {
"types": "./dist/lib/index.d.ts",
"svelte": "./dist/index.js",
"default": "./dist/index.js"
},
"./plugins": {
"types": "./dist/lib/plugins/index.d.ts",
"svelte": "./dist/plugins/index.js",
"default": "./dist/plugins/index.js"
}
},

package.json Outdated
Comment on lines 44 to 75
"@babel/core": "7.23.6",
"@babel/preset-env": "^7.23.6",
"@sveltejs/adapter-auto": "^3.0.0",
"@sveltejs/kit": "^2.0.0",
"@sveltejs/package": "^2.2.3",
"@testing-library/jest-dom": "^6.1.5",
"@testing-library/svelte": "^4.0.5",
"@types/faker": "5",
"@types/jest": "^27.4.1",
"@typescript-eslint/eslint-plugin": "^5.21.0",
"@typescript-eslint/parser": "^5.21.0",
"eslint": "^7.32.0",
"eslint-config-prettier": "^8.5.0",
"eslint-plugin-svelte3": "^3.4.1",
"@types/jest": "^29.5.11",
"@typescript-eslint/eslint-plugin": "^6.14.0",
"@typescript-eslint/parser": "^6.14.0",
"eslint": "^8.55.0",
"eslint-config-prettier": "^9.1.0",
"eslint-plugin-svelte": "^2.35.1",
"faker": "5",
"jest": "^27.5.1",
"prettier": "^2.6.2",
"prettier-plugin-svelte": "^2.7.0",
"svelte-check": "^2.7.0",
"svelte-jester": "^2.3.2",
"svelte-preprocess": "^4.10.6",
"svelte2tsx": "^0.5.9",
"ts-jest": "^27.1.4",
"tslib": "^2.4.0",
"typescript": "^4.7.4",
"vite": "^4.0.3"
"jest": "^29.7.0",
"prettier": "^3.1.1",
"prettier-plugin-svelte": "^3.1.2",
"svelte-check": "^3.6.2",
"svelte-jester": "^3.0.0",
"svelte-preprocess": "^5.1.2",
"svelte2tsx": "^0.6.27",
"ts-jest": "^29.1.1",
"tslib": "^2.6.2",
"typescript": "^5.3.3",
"vite": "^5.0.9"
},
"dependencies": {
"svelte-keyed": "^1.1.6",
"svelte-keyed": "^1.1.7",
"svelte-render": "^1.6.1",
"svelte-subscribe": "^1.0.5"
"svelte-subscribe": "^1.0.6"
},

Choose a reason for hiding this comment

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

Updating all of these dependencies seems out of the scope of this PR. Just pointing this out so that the maintainer is aware.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, we don't want to update all these dependencies in this PR.

However, I'm not sure what the implications are for updating the development dependency to SvelteKit 2.0. Will doing so break compatibility with SvelteKit 1.0?

Copy link

@AdrianGonz97 AdrianGonz97 Dec 17, 2023

Choose a reason for hiding this comment

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

@bryanmylee Fortunately, upgrading to v2 shouldn't break compatibility at all. Likewise, staying on v1 would also be a viable option.

The only necessary changes that are needed to support SvelteKit v2 would be to:

  • update the exports map in package.json (already done)
  • add moduleResolution: "NodeNext" to tsconfig.config
  • update all of the import paths to include file extensions (e.g. import { foo } from "./foo.js") to satisfy the new moduleResolution strategy

Edit: Oh, and updating @sveltejs/package to v2 is a good change as well.

@bryanmylee
Copy link
Owner

Thanks @ryanylee and @AdrianGonz97! The PR helps a lot especially when I haven't had much time to work on this. Just a few issues to resolve, then I'll merge and make a major release update.

@ryanylee
Copy link
Contributor Author

One thing to note is svelte-render needs to also be updated at some point, but the easy workaround for now in this PR is importing from the /createRender file rather than the root index.js

Thank you @AdrianGonz97 for the feedback!

Copy link

@AdrianGonz97 AdrianGonz97 left a comment

Choose a reason for hiding this comment

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

Looks good! Just noticed a few things, but the rest is great 🙂

svelte-render should probably be updated before merging this PR as it's contents are being reexported in src/lib/index.ts, so anything coming from that, like createRender, will have their types all bunged up.

I can open a separate PR for svelte-render to address this issue.

package.json Outdated
Comment on lines 85 to 90
"types": "./dist/lib/index.d.ts",
"svelte": "./dist/index.js",
"default": "./dist/index.js"
},
"./plugins": {
"types": "./dist/lib/plugins/index.d.ts",

Choose a reason for hiding this comment

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

Suggested change
"types": "./dist/lib/index.d.ts",
"svelte": "./dist/index.js",
"default": "./dist/index.js"
},
"./plugins": {
"types": "./dist/lib/plugins/index.d.ts",
"types": "./dist/index.d.ts",
"svelte": "./dist/index.js",
"default": "./dist/index.js"
},
"./plugins": {
"types": "./dist/plugins/index.d.ts",

.npmignore Outdated
@@ -1,7 +1,7 @@
node_modules
/build
/.svelte-kit
/package
/dist

Choose a reason for hiding this comment

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

this doesn't need to be ignored

Suggested change
/dist

@bryanmylee
Copy link
Owner

bryanmylee commented Dec 19, 2023

Updating SvelteKit to 2.x causes conflicts with our peer dependencies for Svelte 3.

Trying to update all my Svelte packages to keep up with SvelteKit has been an absolute shitshow of broken dependencies.

I'm going to spend a day just recreating the entire library along with all the dependent stuff, but this is ridiculous.

@pheuter
Copy link

pheuter commented Dec 20, 2023

It does appear that SvelteKit 2 intentionally drops support for Svelte 3:

We also recommend updating to Svelte 4 first: Later versions of SvelteKit 1.x support it, and SvelteKit 2.0 requires it.

@bryanmylee bryanmylee merged commit 626cccc into bryanmylee:main Jan 2, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants