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: revert breaking change for compiler/preprocess types location #6100

Merged
merged 3 commits into from
Mar 30, 2021

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented Mar 19, 2021

Reverts the relocation of all interfaces in abf11bb
This unintentionally introduced a breaking change in 3.32.0 – this is because all /types are auto-generated, making our source structure the /types distribution structure. Any file renames/relocations become a breaking change as it means that access to the types for X are now somewhere else.

For a single user application, this isn't a huge deal as you can just update a single import. But it poses a bigger issue for third-party dependencies (like the Rollup plugin, or like svelte-preprocess-esbuild) that rely on some minimum version of Svelte as a peer dependency.

In code:

// before (now broken)
import type { PreprocessorGroup, Processed } from 'svelte/types/compiler/preprocess';

// after 
import type { PreprocessorGroup, Processed } from 'svelte/types/compiler/preprocess/types';

PR introduces no functionality changes.


Alternative solution:

We could add export * from './types from the index.ts file, but this means that these types now exist in two locations.

Reverts the change in `interface` locations in abf11bb
No functionality changes.
@dummdidumm
Copy link
Member

I think doing a reexport at index.ts is the better option. It makes it possible to relocate internal structures and not worrying about breaking them. I'd also argue that the deeper import is considered private API and subject to change, so if someone uses that, it's their fault.
In the long run I think it makes sense to use https://github.com/microsoft/rushstack/tree/master/apps/api-extractor , @dominikg has some experience with that I think.

@dominikg
Copy link
Member

thanks for tagging me. I've got to fix the types in vite-plugin-svelte too (currently copied, but they should point to sveltes own and would then also be affected by this)

Unfortunately my experience with api-extractor is limited to reusing the existing setup from vite. An alternative could be tsup https://tsup.egoist.sh/#generate-declaration-file . I agree that having an index that reexports public api is a good approach.

@lukeed
Copy link
Member Author

lukeed commented Mar 22, 2021

I'd also argue that the deeper import is considered private API

It's 100% public API since it's the only location/way to access these types. It's best to make all /compiler/* types accessible on svelte/compiler & re-export from internal files. This allows internals to restructure as needed & the public face is always consistent. If types are free to move around at any point in time, then versioning loses meaning & it's near-impossible for a third-party to establish a peer dependency.

That said, a change like ^^ cannot happen until a 4.0 release (if we're to only maintain a single access point) because it'd still be breaking.

I'll update the PR to re-export types, since they've lived at their new address for 3 minor-versions & there may now be dependents relying on the new location.

@dummdidumm
Copy link
Member

I'd also argue that the deeper import is considered private API

It's 100% public API since it's the only location/way to access these types.

If they are reexported from the index, they should be imported from there - at least that's what I'm argueing.

It's best to make all /compiler/* types accessible on svelte/compiler & re-export from internal files. This allows internals to restructure as needed & the public face is always consistent.

I agree with that. Do you have an idea how to achieve that? Right now if you reexport them I think both locations will be part of the public api unless we cleanup the types afterwards in some way.

@lukeed
Copy link
Member Author

lukeed commented Mar 22, 2021

If they are reexported from the index, they should be imported from there - at least that's what I'm argueing.

Yes, this is the change I just made, but only to accommodate the fact that there have been 3 minor releases w/ types in the new location. The original contents of this PR would be breaking for any new imports to the new surprise location.

Do you have an idea how to achieve that?

I manage my definition files manually because (D)TS tooling isn't worth the headspace. The best I've seen is rollup-plugin-dts which (iirc) can combine types from multiple *.ts files into a single .d.ts output. That would mean there'd be a types/compiler/preprocess.d.ts only.

But again – we cannot do this until 4.0. It will be a breaking change.

@Conduitry Conduitry merged commit e5a5eae into master Mar 30, 2021
@Conduitry Conduitry deleted the fix/preprocess branch March 30, 2021 02:40
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