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(types): extend CompileOptions interface directly #126

Merged
merged 4 commits into from
Oct 19, 2020

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented Aug 28, 2020

Extends the CompileOptions directly, since everything gets passed into svelte/compiler anyway. Saves us from having an ongoing battle of "oh this is missing too", as we have.

This is separate to (and should land after) #125

Verified that both PRs work together locally.

@benmccann
Copy link
Member

I think you can remove the generate option as being inherited as well: https://svelte.dev/docs#svelte_compile

@lukeed
Copy link
Member Author

lukeed commented Aug 28, 2020

The CompileOptions#generate is string | false -- we're being more specific here so I kept it

@benmccann
Copy link
Member

I sent sveltejs/svelte#5321 to update CompileOptions#generate

@lukeed lukeed requested a review from benmccann August 31, 2020 00:09
@Conduitry
Copy link
Member

This makes me nervous because we're now making that type existing at that path in the svelte package a secret part of its API. Do we have somewhere to document this? How do we know not to move or rename that as part of a refactor?

@lukeed
Copy link
Member Author

lukeed commented Sep 8, 2020

That's already been the case. Shuffling/renaming exported type interfaces should always be considered as part of semver.

@lukeed
Copy link
Member Author

lukeed commented Sep 8, 2020

TBH I don't really like Svelte's current system of exporting types in general. Largely because of the same concerns you have. When they're auto-generated by TS, you don't have as much fine-tune control of what ends up where.

A simple(r) way to resolve this in the future is to export everything from svelte directly – that way it doesn't matter as much where/which specific files each definition is coming from. But as it is, all files are accessible and so any current exports are already part of the contract. (This is a big reason for native ESM's exports mapping: "you get to touch this & that only")

@Conduitry
Copy link
Member

This is actually already a breaking change, because types/compiler/interfaces.d.ts didn't exist in the published package until 3.5.0, and this plugin says it works back to 3.0.0 (and indeed, says that it still works with Svelte 2, although I don't know whether we're already publishing incorrect types for Svelte 2's compiler options).

Given that, I'd probably be in favor of getting our exports in order in Svelte - including the types we think it makes sense to expose directly at the root, rather than from some file deep in the package - and then coming back to this, consuming the new types and making it a breaking change. I'm not sure what to do about Svelte 2. At some point we should probably drop it from this plugin, perhaps at the same time.

@lukeed
Copy link
Member Author

lukeed commented Oct 6, 2020

We should decide what we're doing here. Would love to finally get rid of the intellisense errors that decorate all my rollup-plugin-svelte usage – especially since our sveltejs/template includes (basic) options that the current types don't agree with.

@lukeed lukeed mentioned this pull request Oct 10, 2020
@antony antony added this to the 7.0.0 milestone Oct 14, 2020
@lukeed
Copy link
Member Author

lukeed commented Oct 19, 2020

#138 solves the concern re: [email protected] types structure. In future we'll set up a better/stricter public-facing types system so that it no longer becomes a concern. In the age of TS, type definitions are now part of the package contract.

@lukeed lukeed merged commit 1871d5f into master Oct 19, 2020
@lukeed lukeed deleted the types/options-extend branch October 19, 2020 19:20
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.

4 participants