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

[Experimental] Add Typekit for @typespec/http #5340

Merged
merged 5 commits into from
Jan 24, 2025

Conversation

joheredi
Copy link
Member

This PR includes:

  • Consolidating the format used to define Typekits
  • Adding @typespec/compiler/experimental/typekit sub export
  • Adding @typespec/http/experimental/typekit sub export and implementation

Note: experimental/typekit subpaths were added for improved UX. The typekit import has a side effect which is augmenting existing typekit so having it within its own export isolates the side effect and makes it easier consumers to migrate once it becomes stable into @typespec/compiler/typekit and @typespec/http/typekit respectively

@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 12, 2024

All changed packages have been documented.

  • @typespec/compiler
  • @typespec/http
Show changes

@typespec/compiler - feature ✏️

Add Experimental Typekit helpers for @typespec/http

@typespec/http - feature ✏️

Add Experimental Typekit helpers for @typespec/http

@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 12, 2024

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs 🛝 VSCode Extension

@witemple-msft
Copy link
Member

What does a library consumer need to do to activate the new typekit functionality? I am a little weirded out that there doesn't seem to be any import from experimental/typekit from the HTTP library in the tests, so are these new typekits activated automatically on any import of @typespec/http? i also don't see the experimental export path from the HTTP library, only the compiler core.

I think this change is righteous, just want to make sure we have the consumption pattern nailed down for these experimental APIs.

I also worry a little bit about discoverability and how someone is meant to know that import "@typespec/http/experimental/typekit"; ought to be required for its side effects alone to make these extensions available. In the compiler core, you actually have to import $ from the side-effecting path, but here there won't be anything to import, you'll just import nothing from the module and it'll change the behavior of $. Do I have all this right?

@joheredi joheredi force-pushed the feature/experimental-http-typekit branch from 5ac71ee to a501795 Compare January 14, 2025 23:23
@joheredi
Copy link
Member Author

What does a library consumer need to do to activate the new typekit functionality? I am a little weirded out that there doesn't seem to be any import from experimental/typekit from the HTTP library in the tests, so are these new typekits activated automatically on any import of @typespec/http? i also don't see the experimental export path from the HTTP library, only the compiler core.

I think this change is righteous, just want to make sure we have the consumption pattern nailed down for these experimental APIs.

I also worry a little bit about discoverability and how someone is meant to know that import "@typespec/http/experimental/typekit"; ought to be required for its side effects alone to make these extensions available. In the compiler core, you actually have to import $ from the side-effecting path, but here there won't be anything to import, you'll just import nothing from the module and it'll change the behavior of $. Do I have all this right?

I missed adding the export for @typespec/http/experimental/typekit, fixed it.

Outside of @typespec/http, users need to explicitly import "@typespec/http/experimental/typekit" for the side effect to apply. Within @typespec/http, the augmentation is globally available because TypeScript integrates augmentations into the package's type space during compilation.

And yes, you are right, the import "@typespec/http/experimental/typekit" is purely for its side effect of augmenting types, and nothing needs to be imported explicitly from the module for the behavior to take effect.

The concern about discoverability is valid—it's something we should address. Perhaps we can do it through the documentation or provide a more explicit activation path. I am open to suggestions if you have any thoughts on how we can better streamline this!

@joheredi
Copy link
Member Author

@witemple-msft I dug deeper because the runtime side effects being available without explicitly loading the module was bugging me. It turns out createTestHost is the reason—it loads all modules in the library's packageRoot when HttpTestLibrary is registered. Without HttpTestLibrary, the module isn’t loaded, and the side effects are unavailable. Let me know if this clears things up!

I will add an explicit import to avoid confusion down the road

allenjzhang
allenjzhang previously approved these changes Jan 23, 2025
@allenjzhang allenjzhang dismissed their stale review January 23, 2025 16:42

Incomplete review

@witemple-msft
Copy link
Member

The concern about discoverability is valid—it's something we should address. Perhaps we can do it through the documentation or provide a more explicit activation path. I am open to suggestions if you have any thoughts on how we can better streamline this!

I don't think we need to ponder this too much. When TypeKit is stabilized and then these specific extensions are stabilized, it will presumably be registered by any use of the HTTP library and that will be a good story.

Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

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

This very virtuous addition looks great to me and I can't wait to try it in http-server-js and give some practical feedback on the API and add more extensions that we might need.

@joheredi joheredi force-pushed the feature/experimental-http-typekit branch from 9e845fd to f6ca5ec Compare January 24, 2025 19:43
@joheredi joheredi enabled auto-merge January 24, 2025 19:44
@joheredi joheredi added this pull request to the merge queue Jan 24, 2025
Merged via the queue into microsoft:main with commit 2389dc7 Jan 24, 2025
24 checks passed
@joheredi joheredi deleted the feature/experimental-http-typekit branch January 24, 2025 20:28
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.

6 participants