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

Export store's useful typescript definitions - close #5864 #5887

Merged
merged 12 commits into from
Mar 24, 2021
Merged

Export store's useful typescript definitions - close #5864 #5887

merged 12 commits into from
Mar 24, 2021

Conversation

sangxxh
Copy link
Contributor

@sangxxh sangxxh commented Jan 16, 2021

Close #5864

The store has some internal types that are useful and should be exported to help consumer apps typing their store implementations. See more in #5864

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.
  • 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

the store has some internal types that are useful and should be exported
to help consumer apps typing their store implementations.
Copy link
Contributor

@dreitzner dreitzner left a comment

Choose a reason for hiding this comment

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

Exports "missing" store types. If there are no side effects that are not know to me this should be great.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

The same was already proposed in #5170, and @Conduitry said that he doesn't want to expose Invalidator and StartStopNotifier as these are internal API. So we would at least need to remove these two from the exports.

@sangxxh
Copy link
Contributor Author

sangxxh commented Jan 21, 2021

The same was already proposed in #5170, and @Conduitry said that he doesn't want to expose Invalidator and StartStopNotifier as these are internal API. So we would at least need to remove these two from the exports.

I have removed the exports for those 2 types.

Also trying to have some careful thought about the other types of the store. On one hand, they're helpful, on the other hand, I can get around the limitation of not exporting them by writing code as such:

/**
 * Instead of:
 * getCoinList: StartStopNotifier<ReadonlyArray<CoinMarketData>> = (
 * 	set: Subscriber<Date>
 * ) => {
 * 	...
 * 
 * 	set(...)
 * }
 *
 * Write like this:
 */
async function getCoinList(): Promise<ReadonlyArray<CoinMarketData>> {
	const res = await fetch(
		'https://api.coingecko.com/api/v3/coins/markets?vs_currency=usd'
	);

	if (!res.ok) {
		throw new Error(await res.text());
	}

	const markets = await res.json();
	return createFormattedCurrencyFields(markets);
}


export const coinMarket: Readable<CoinMarketDataStore> = readable(
	initialValue,
	(set) => {
		(async () => {
			try {
				/**
				 * Don't need to call getCoinList(set) 
				 */
				const markets = await getCoinList();
				set({
					markets,
					error: null,
				});
			} catch (error) {
				set({
					markets: [],
					error,
				});
			}
		})();

		return function stop() {
			// nothing
		};
	}
);

What do you think? Does it look like this is the recommended approach rather than exporting the types or we should still export those types for other use cases?

@dummdidumm
Copy link
Member

I gotta reverse my opinion on StartStopNotifier. This one is okay to export, too, it's public API and it's in the docs - you even must provide that parameter to readable, so ..

@the-homeless-god
Copy link

@dummdidumm could you tell what blocks this PR? let's rebase it and merge, why we're awaiting from January?

@dummdidumm dummdidumm merged commit 18d9fb5 into sveltejs:master Mar 24, 2021
@sangxxh sangxxh deleted the feat/export-store-types branch March 24, 2021 19:31
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.

Can Svelte export all store's Typescript definitions?
6 participants