Skip to content

fix: store types and some other internal types that got lost in the conversion #8658

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
Merged
merged 4 commits into from
May 30, 2023
Merged

fix: store types and some other internal types that got lost in the conversion #8658

merged 4 commits into from
May 30, 2023

Conversation

gtm-nayan
Copy link
Contributor

HEADS UP: BIG RESTRUCTURING UNDERWAY

The Svelte repo is currently in the process of heavy restructuring for Svelte 4. After that, work on Svelte 5 will likely change a lot on the compiler aswell. For that reason, please don't open PRs that are large in scope, touch more than a couple of files etc. In other words, bug fixes are fine, but feature PRs will likely not be merged.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@vercel
Copy link

vercel bot commented May 29, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-dev-2 ❌ Failed (Inspect) May 30, 2023 11:05am

@gtm-nayan gtm-nayan changed the base branch from master to version-4 May 29, 2023 14:42
dominikg
dominikg previously approved these changes May 29, 2023
View reviewed changes
benmccann
benmccann reviewed May 29, 2023
View reviewed changes
packages/svelte/src/runtime/store/index.js Outdated
@@ -26,7 +26,7 @@ export function readable(value, start) {
* Create a `Writable` store that allows both updating and reading by subscription.
* @template T
* @param {T} value initial value
Copy link
Member

Choose a reason for hiding this comment

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

I think this is also optional in master if I understand that's what the = syntax means. Do we want to do the same here?

svelte/src/runtime/store/index.ts

Line 68 in 3bc791b

* @param {*=}value initial value

Copy link
Member

Choose a reason for hiding this comment

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

both ways work, {string=} foo is google closure syntax while {string} [foo] is from jsdoc itself. ts knows both https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#param-and-returns I think being consistent is important, using the [] syntax is a bit more to type but closer to jsdoc and allows telling about a default values.

@benmccann
Copy link
Member

It looks like there are others that got lost in the migration. E.g.

svelte/packages/svelte/src/runtime/internal/dev.js

Line 60 in 79f9d8f

* @param {Node} anchor

I found some with egrep -r "export.*\?:" src/

@benmccann
Copy link
Member

some others:

svelte/packages/svelte/src/compiler/preprocess/index.js

Line 335 in 79f9d8f

export default async function preprocess(source, preprocessor, options) {

https://github.com/sveltejs/svelte/blob/version-4/packages/svelte/src/compiler/utils/link.js

svelte/packages/svelte/src/runtime/motion/tweened.js

Line 62 in 79f9d8f

function set(new_value, opts) {

svelte/packages/svelte/src/runtime/internal/Component.js

Line 224 in 79f9d8f

m: function mount(target, anchor) {

svelte/packages/svelte/src/runtime/internal/transitions.js

Line 80 in 79f9d8f

export function transition_out(block, local, detach, callback) {

also noticed that unknown got convert to any, so that could be another thing to check for:

svelte/packages/svelte/src/compiler/compile/render_dom/invalidate.js

Line 97 in 79f9d8f

export function renderer_invalidate(renderer, name, value, main_execution_context = false) {

it also seems some types got dropped like in

svelte/packages/svelte/src/runtime/internal/dom.js

Line 1006 in 79f9d8f

export function custom_event(type, detail, { bubbles = false, cancelable = false } = {}) {

and it looks like the type may have really changed here:

svelte/packages/svelte/src/compiler/utils/extract_svelte_ignore.js

Line 24 in 79f9d8f

export function extract_svelte_ignore_from_comments(node) {

@dominikg dominikg dismissed their stale review May 30, 2023 11:18

too many changes since then

@gtm-nayan gtm-nayan changed the title fix: store types fix: store types and some other internal types that got lost in the conversion May 30, 2023
benmccann
benmccann approved these changes May 30, 2023
View reviewed changes
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

lgtm. though I'm scared of what else we might be missing

i'm also scared that our type checking didn't catch any of these errors. Is that because none of these are actually used in an optional manner or because we're just way too loose with our type checking settings?

@benmccann benmccann merged commit fe06a39 into version-4 May 30, 2023
@benmccann benmccann deleted the store-types branch May 30, 2023 19:17
@benmccann benmccann mentioned this pull request May 30, 2023
Merged
@gtm-nayan gtm-nayan mentioned this pull request Jun 17, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikg dominikg dominikg left review comments

@benmccann benmccann benmccann approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@gtm-nayan @benmccann @dominikg