This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document themon our wiki's API Breaking Changes page.

Also, please make sure@DanielRosenwasser and@RyanCavanaugh are aware of the changes, just as a heads up.

@microsoft-github-policy-service agree

@typescript-bot test it

Starting jobs; this comment will be updated as builds start and complete.

Hey@jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@jakebailey Here are the results of running the user tests comparingmain andrefs/pull/58640/merge:

Everything looks good!

@jakebailey
The results of the perf run you requested are in!

I would not be surprised if TypeScript's lack of support is the main reason why I haven't seen any real usage of this feature yet.

I don't know. I feel like, if I, as a library writer in control of my own API, have the choice between telling my users either:

1. You have to writeimport { "Foo.new" as Foo_new } from "foo" (a renaming import) for each thing you want to import from this library, -OR-
2. You can just writeimport { Foo_new } from "foo" if you want.

Then I'm going to choose to tell them 2) every time. If I know a plurality of my consumers are importing from JavaScript then I'm only going to tell them 1) if there's absolutely no way around it.

@jakebailey Here are the results of running the top 400 repos comparingmain andrefs/pull/58640/merge:

Everything looks good!

Yes, if your API would be improved by not using this feature than you obviously shouldn't be using it. My point was that if you have a use case for which this feature is the most natural fit (see the real-world example I posted), you're still effectively prevented from using it because most people use VSCode these days and the TypeScript language service hasn't added support for it yet.

Then I'm going to choose to tell them 2) every time. If I know a plurality of my consumers are importing from JavaScript then I'm only going to tell them 1) if there's absolutely no way around it.

I don't think that's the point. One of the issues related to this PR,#40594, mentions this feature is for WASM support, since WASM can export unicode exports.

Typescript also prides itself on being a superset of Javascript, so since Javascript supports this type of imports, Typescript should too. This PR does not encourage library authors to make their API more inconvenient to use, but there are valid use cases for it.

@csvn I’m not arguing against the feature by any means - I was specifically responding to the OP’s conjecture that people might not be using the featurein JavaScript because TS doesn’t support it, which didn't make sense to me because if you have to use a quoted name in an import then there’s probably no other alternative, while if you’re writingexports for JS, there’s no reason to use it on purpose as it just makes things harder for consumers for no reason.

Completions inimport {} andexport {} lists (it's unclear if this is necessary and/or how exactly this should work given that the module namespace identifiers can be, well, arbitrary)

Spitballing, but these should be

• supported as string literal completions
• converted into string literal completions if writing an identifier that matches and the client supports insertText
  • insert anas if the name is strictly being used for a localimport (not re-exporting)

Feel free to add that in this PR, but I wouldn't gate the PR on that at all.

Ignoring what Daniel already said, I can't really find a fault with the PR as-is.

Is this something we're trying to get into 5.5 RC, or wait for 5.6?

Feel free to add that in this PR, but I wouldn't gate the PR on that at all.

Yes, I strongly suggest this PR should be done without fine-tuning everything. From my experience of developing#49297, I tried to make all the language service stuff great at once, and there was too much work to do and I drained.

I had a checklist for my PR, that's everything I can think of about this feature.

• Code fix: unnecessary string namesexport { a as "b" } =>export { a as b }
• Special case:export { a as "export=" } reportsAn export assignment cannot be used in a module with other exported elements.ts(2309)
• Internal refactor:InternalSymbolName?,CompletionDetailsFull?,SignatureHelp
• Completion
  • type z = typeof q.^
  • { V as '' } (empty name does not show up)
  • export { V as 'export=' } (this name does not show up)
• Find reference:ns['b c']
• Rename
  • export { ^a } (identifier to string)
  • export { a as ^b } (identifier to string)
  • export { a as "b" } (remove unnecessary quotes)
  • export * as "b c" (string rename)
  • export * as x (identifier to string)
  • import { a } (identifier to string)
  • import { ^c as b } (identifier to string)
  • import { "^c" as b } (string rename)
  • ns['a b'] => ns['d'] (rename to identifier not removing quotes)
  • ns.b (identifier to string)
• Hover: generates invalid code

Thanks for the heads up about theexport= edge case—I wasn't aware of that one. Arbitrary module namespace identifiers causing name collisions with internal symbols is something I had to deal with in esbuild's implementation too (I wasusing"*" as a special name before I had to change it). It is a strange edge case, but luckily it doesn't block usage of this feature other than for that specific export alias.

It looks like the way to deal with that in the TypeScript code is to add double underscores before the internal symbol name (so maybe__export=?). That seems to work but it touches a lot of files (mostly test baselines) and has other consequences (e.g. potentially affecting error messages) so maybe it's best done in a follow-up PR? I'm happy to put a PR up for that too if that's a change that the TypeScript team is open to.

Spitballing, but these should be

• supported as string literal completions
• converted into string literal completions if writing an identifier that matches and the client supports insertText
  • insert anas if the name is strictly being used for a localimport (not re-exporting)

Feel free to add that in this PR, but I wouldn't gate the PR on that at all.

I added support for the first one (completion within an existing string literal). I'm not sure whether or not I have all of the necessary context about the code to add support for the second one.

Ok now I've also added support for the second one (complete identifiers into matching string literals, usingas when appropriate). But I'm less sure about this change. Is it ok to always return a non-identifier alias as a completion like that? I'm happy to roll that change back and leave it for future work if the correct approach is indeed more complicated than what my solution does.

This looks good but will need to wait to merge until after we switchmain to target 5.6 next week. I'm going to modify the PR description to useFixes #<issue> for each issue instead of the bulleted list since GH's issue linking and auto-close functionality doesn't understand bulleted lists.

The TypeScript team hasn't accepted the linked issue#40594. If you can get it accepted, this PR will have a better chance of being reviewed.

@typescript-bot perf test
@typescript-bot run dt
@typescript-bot test top400
@typescript-bot test tsserver top100
@typescript-bot user test this
@typescript-bot user test tsserver

Starting jobs; this comment will be updated as builds start and complete.

Hey@rbuckton, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@rbuckton Here are the results of running the user tests comparingmain andrefs/pull/58640/merge:

Everything looks good!

@rbuckton Here are the results of running the user tests comparingmain andrefs/pull/58640/merge:

Something interesting changed - please have a look.

Server exited prematurely with code unknown and signal SIGABRT

{"seq":11277,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/packages/effect/test/STM.test.ts","line":398,"offset":1,"entryNames":["assert"]}}{"seq":11278,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/packages/effect/test/STM.test.ts","line":405,"offset":14}}{"seq":11279,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/packages/effect/test/STM.test.ts","line":409,"offset":11}}{"seq":11280,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/packages/effect/test/STM.test.ts","line":422,"offset":58}}

#!/bin/bashgit clone https://github.com/Effect-TS/effect.git --recurse-submodulesgit -C"./effect" reset --hard 2c2280b98a11fc002663c55792a4fa5781cd5fb6pnpm --dir"/mnt/ts_downloads/base/effect" install --no-frozen-lockfile --prefer-offline --ignore-scripts --reporter=silentdownloadUrl=$(curl -s"https://typescript.visualstudio.com/TypeScript/_apis/build/builds/162005/artifacts?artifactName=RepoResults4&api-version=7.0"| jq -r".resource.downloadUrl")wget -O RepoResults4.zip"$downloadUrl"unzip -p RepoResults4.zip RepoResults4/effect.replay.txt> effect.replay.txtnpm install --no-save @typescript/server-replay

# `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.npx tsreplay ./effect ./effect.replay.txt<PATH_TO_tsserver.js>

@rbuckton
The results of the perf run you requested are in!

@rbuckton Here are the results of running the top 400 repos comparingmain andrefs/pull/58640/merge:

Everything looks good!

@rbuckton Here are the results of running the top 200 repos comparingmain andrefs/pull/58640/merge:

Something interesting changed - please have a look.

Server exited prematurely with code unknown and signal SIGABRT

{"seq":135,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/packages/prisma/seed-insights.ts","line":499,"offset":9,"entryNames":["arguments"]}}{"seq":136,"type":"request","command":"definitionAndBoundSpan","arguments":{"file":"@PROJECT_ROOT@/packages/prisma/seed-insights.ts","line":506,"offset":42}}{"seq":137,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/packages/types/EventManager.d.ts"],"openFiles":[]}}{"seq":138,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/packages/lib/weekday.test.ts","projectRootPath":"@PROJECT_ROOT@"}]}}

#!/bin/bashgit clone https://github.com/calcom/cal.com --recurse-submodulesgit -C"./cal.com" reset --hard 3b1de344515c525a20833232e3aed0c04c273f44yarn --cwd"/mnt/ts_downloads/base/cal.com" install --no-immutable --mode=skip-builddownloadUrl=$(curl -s"https://typescript.visualstudio.com/TypeScript/_apis/build/builds/162008/artifacts?artifactName=RepoResults7&api-version=7.0"| jq -r".resource.downloadUrl")wget -O RepoResults7.zip"$downloadUrl"unzip -p RepoResults7.zip RepoResults7/calcom.cal.com.replay.txt> calcom.cal.com.replay.txtnpm install --no-save @typescript/server-replay

# `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.npx tsreplay ./cal.com ./calcom.cal.com.replay.txt<PATH_TO_tsserver.js>

Server exited prematurely with code unknown and signal SIGABRT

{"seq":771,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/packages/babel-runtime-corejs3/helpers/esm/regeneratorRuntime.js","line":233,"offset":23,"includeExternalModuleExports":false,"triggerKind":2,"triggerCharacter":""}}{"seq":772,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/packages/babel-runtime-corejs3/helpers/esm/regeneratorRuntime.js","line":251,"offset":15,"includeExternalModuleExports":false,"triggerKind":1}}{"seq":773,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/packages/babel-runtime-corejs3/helpers/esm/regeneratorRuntime.js","line":251,"offset":15,"entryNames":["_forEachInstanceProperty"]}}{"seq":774,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/packages/babel-runtime-corejs3/helpers/esm/regeneratorRuntime.js","line":259,"offset":23}}

#!/bin/bashgit clone https://github.com/babel/babel --recurse-submodulesgit -C"./babel" reset --hard 90fdd7e11deff47296478f8cdd77de07c51fcaeedownloadUrl=$(curl -s"https://typescript.visualstudio.com/TypeScript/_apis/build/builds/162008/artifacts?artifactName=RepoResults3&api-version=7.0"| jq -r".resource.downloadUrl")wget -O RepoResults3.zip"$downloadUrl"unzip -p RepoResults3.zip RepoResults3/babel.babel.replay.txt> babel.babel.replay.txtnpm install --no-save @typescript/server-replay

# `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.npx tsreplay ./babel ./babel.babel.replay.txt<PATH_TO_tsserver.js>

@rbuckton Here are some more interesting changes from running the top 200 repos suite

Server exited prematurely with code unknown and signal SIGABRT

{"seq":803,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/x-pack/plugins/task_manager/server/buffered_task_store.mock.ts","line":8,"offset":39,"includeExternalModuleExports":false,"triggerKind":1}}{"seq":804,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/x-pack/plugins/task_manager/server/buffered_task_store.mock.ts","line":8,"offset":39,"entryNames":["@kbn/utility-types"]}}{"seq":805,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/x-pack/test/api_integration/services/transform.ts"],"openFiles":[]}}{"seq":806,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/x-pack/plugins/stack_alerts/common/comparator_types.ts","projectRootPath":"@PROJECT_ROOT@"}]}}