Update type-only import semantics to allow type queries #36092
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewbranch
commented
Jan 9, 2020
andrewbranch
requested review from
sandersn,
DanielRosenwasser,
RyanCavanaugh and
weswigham
sandersn
approved these changes
Jan 9, 2020
There was a problem hiding this comment.
The change looks OK to me, but with the caveat the I didn't review the original code so this is the first time for me to look at it. @weswigham should probably answer your question about whether there's a better way to finagle const enum references.
|
@typescript-bot perf test this |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 124dcd6. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@andrewbranch Here they are:Comparison Report - master..36092
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check time appears up ~2–4%. I think this discussion will have some bearing on what we can do, so I’ll hold off on doing anything specifically for perf until we make some other decisions. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
andrewbranch
changed the title
andrewbranch
changed the title
This comment has been minimized.
This comment has been minimized.
This was referenced Jan 11, 2020
|
For a moment I thought we could also allow type-only imports in class property declarations without initializers: class C {
[onInit]: any;
}But that’s actually a problem for ES class field semantics if I understand correctly. It’s also maybe worth noting that at this point, it would be very easy to allow people to use const enums as type-only imports (#36003), even though I think it’s pretty unintuitive. |
|
Yeah, that's definitely an error. Here's another fun case now that you mention non-emitted members in classes: import type { onInit } from "./framework-hooks";
abstract class Component {
abstract [onInit](): void;
} |
|
Edit: NodeFlag approach was terrible for incremental parsing. Reverted. |
|
@typescript-bot perf-test this |
andrewbranch
force-pushed
the
type-only-2
branch
from
a0691da
to
a5ca492
Compare
|
Looks like there's diagnostic conflicts |
|
@typescript-bot perf test this again for good measure please |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at be5f50f. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@andrewbranch Here they are:Comparison Report - master..36092
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
RyanCavanaugh
approved these changes
Jan 23, 2020
|
Is there any settings could disable this?? It's so annoying when use DI framework like nest.js. I want it always import value not type.
it will raise runtime error for imports statement likes this, and hard to catch during build. |
|
It doesn’t do this unless you set |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #35200, a type-only import or export created a synthetic symbol which reflected only the type-side meaning of its target. This meant that the imported names were unusable in any value position. This PR removes the intermediate synthetic symbol and instead issues an error if the imported names appear in any non-ambient expression context. Namely, this allows for:
One oddity that arises with this approach is how to treat a namespace import that includes type-only exports:
Essentially, there is a limitation that if you need to access the value meaning of a type-only export, you can’t do so through a namespace import—you have to use a named (or default) import.
The distinction is subtle and confusing, but I think internally consistent, and unlikely to come up much in real code. At least, I think it’s better to start with this limitation than to be overly permissive in a way that could make the type system misrepresent the reality of the shape of a module.
Fixes #36040
Fixes #36003 (the part I said was a bug)
Fixes #36004