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

Retain reexports in js emit even when they export nothing #37124

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

weswigham
Copy link
Member

Fixes #37123

@weswigham
Copy link
Member Author

cc @rbuckton (since you're the last one to interact with this ellision code) and @andrewbranch (since this is tangentially related to your import type stuff) - I'd like reviews, but GH reviewer suggestions are, as normal, slow~

@andrewbranch
Copy link
Member

andrewbranch commented Mar 2, 2020

This is a breaking change, right? This feels exactly the same as import { Type } from './a' or import * as types from './moduleThatOnlyExportsTypes' to me, and the behavior for those is to elide unless --importsNotUsedAsValues is preserve or error. We decided not to make that the default behavior because it would have meant that a user could break their program at runtime by upgrading their TypeScript version (by including modules that weren’t included before or by changing the module order). This is in the same boat, right?

@weswigham
Copy link
Member Author

Sortof - in this case, I found our behavior super surprising for export *; it's impossible to make an export * "locally used as a value", so unlike the others, it's looking at the shape of the target, which makes it type directed. It's very much against our design philosophy.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

I think this makes sense, but it should probably still be noted as a potentially breaking change, albeit with quite small chances of actually breaking anything.

@weswigham weswigham added the Breaking Change Would introduce errors in existing code label Mar 2, 2020
@weswigham
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 2, 2020

Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 372d6e8. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 2, 2020

Heya @weswigham, I've started to run the extended test suite on this PR at 372d6e8. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@weswigham weswigham merged commit af4201f into microsoft:master Mar 2, 2020
@weswigham weswigham deleted the retain-export-stars branch March 2, 2020 22:33
@NickColley
Copy link

NickColley commented May 15, 2020

This is a breaking change, right?

Hello, just so you are aware, this change did break our project.

Please consider releasing TypeScript in alignment with semantic versioning in the future.

Thanks and take care 💚

Financial-Times/dotcom-page-kit#816

@stephenzsy
Copy link

would that be possible to have a flag turn off the always reexport even there is nothing? Some times they are .d.ts files and the bundler do not bundle them for browser side code. This would prevent us from upgrading to 3.9 or later versions

@jkieboom
Copy link

This also broke for our project. In our case we export * a .d.ts file, which will fail later at runtime because there is no actual js module. A compiler error for that would have been nice.

@weswigham
Copy link
Member Author

There's no error for that because a module .d.ts file is supposed to correspond to a module at runtime. At least that's how our own emit works. That's why the declaration file's presence lets you write the import or reexport (with a js extension, even!) in the first place. We probably should consider a export type * from "..." to mirror import type, however. I think we may have an issue for that somewhere...

@narkowicz
Copy link

This also broke our projects due to .d.ts files. We're left with a require to a local path that doesn't exist in the compiled output. In lieu of a major version bump, a compilation warning would have been really valuable.

The compilation behaviour now seems inconsistent given that we can still re-export named types from a .d.ts file without affecting compiled JavaScript.

FWIW we have been using .d.ts in a way not strictly intended- these files are compiled from JSON Schemas and define a heap of external JSON data types, rather than JavaScript modules. Collections of these types are re-exported along with a suite of type guards etc. for consumption in other TS packages.

We've had to roll back to 3.8.3 until we can export type * from "..." which would cover our use case well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

export * from "mod" declarations are elided when "mod" has no exports
7 participants