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

fix: allow writable/readable empty initialization #6293

Merged
merged 10 commits into from
May 20, 2021
Merged

fix: allow writable/readable empty initialization #6293

merged 10 commits into from
May 20, 2021

Conversation

ignatiusmb
Copy link
Member

@ignatiusmb ignatiusmb commented May 4, 2021

Fixes #6291
Fixes #6345

Both writable and readable initialized without any arguments are already valid, but TS complains about it. This makes both allowed to be emptily initialized. A special exception to readable, although it's valid to use empty initialization (not sure why anyone would do this), when the first argument is passed, the callback is required to be defined as well (according to the docs).

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
  • This message body should clearly illustrate what problems it solves.

Copy link

@dodiameer dodiameer left a comment

Choose a reason for hiding this comment

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

Looks like there aren't any problems with the changes!

@ignatiusmb
Copy link
Member Author

This should now fix #6345 as well

@dummdidumm
Copy link
Member

dummdidumm commented May 20, 2021

I think you need to update the docs some more, right now they tell you that the second argument is required, which would be no longer the case. Other than this this looks good 👍

@ignatiusmb
Copy link
Member Author

Oops, nice catch. Done!

@dummdidumm
Copy link
Member

Just to be extra safe: Could you add

  • one test for a writable without an arg
  • one test for a readable without an arg
  • one test for a readable without an updater

to https://github.com/sveltejs/svelte/blob/master/test/store/index.js ? That would be great.

@ignatiusmb
Copy link
Member Author

Good idea, done!


I was confused why npm run test -- -g store didn't produce the output, I took a look at GitHub pipelines as well and there's no sign of store. Turns out the test only globs .ts indexes, so I renamed it (seems to be the only one left behind as well).

const test_folders = glob('*/index.ts', { cwd: 'test' });

@dummdidumm dummdidumm merged commit b295d68 into sveltejs:master May 20, 2021
@dummdidumm
Copy link
Member

dummdidumm commented May 20, 2021

Thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants