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

Export and use utility functions from lib/utils #5643

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

steverice
Copy link
Contributor

Export createStateSymbol from lib/utils, and export all of utils in @typespec/compiler so they can be used by other packages.

Additionally, replace duplicate definitions of utils functions in @typespec/compiler with imports from utils.

Export `createStateSymbol` from `lib/utils`, and export all of utils in `@typespec/compiler` so they can be used by other packages.

Additionally, replace duplicate definitions of `utils` functions in `@typespec/compiler` with imports from `utils`.
@microsoft-github-policy-service microsoft-github-policy-service bot added the compiler:core Issues for @typespec/compiler label Jan 16, 2025
@@ -1,7 +1,7 @@
import type { Model, ModelProperty, Type } from "../core/types.js";
import { unsafe_useStateMap, unsafe_useStateSet } from "../experimental/state-accessor.js";

function createStateSymbol(name: string) {
export function createStateSymbol(name: string) {
Copy link
Member

Choose a reason for hiding this comment

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

we actually wouldn't want to expose that outside of the compiler. This function generate a unique id for state symbols for the compiler only. For your own library you should use the already exposed sytem for defining state using createLibrary (Pass stateKeys and get the generated sysmbols form the created library object)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out — agreed that we don't want to export createStateSymbol outside of @typespec/compiler.

I was adding it as an export so that we could use it in decorators.ts and deprecation.ts since compiler does not define its own "library" with createTypeSpecLibrary. Which of these alternatives do you think would be best?

  • re-implement createStateSymbol inside of decorators.ts and deprecation.ts (the status quo ante)
  • do define a "TypeSpec" library using createStateSymbol inside of compiler, and treat all of compiler's decorators as a part of that
  • explicitly specify the exports from utils.ts in index.ts, so that we can export createStateSymbol for use within compiler but not export it outside of the module
  • split up utils.ts into separate files, one of which contains internal-to-compiler utils and the other utility functions that should be exported

Either of the latter two solutions would also address whether we export useStateMap and useStateSet from compiler, which @bterlson suggested we probably don't want to do.

Copy link
Member

Choose a reason for hiding this comment

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

I think 3 or 4 sounds good but we do already expose those useStateMap and useStateSet as unsafe_ version for now if I remember correctly.

Is there anything else you wanted to export from there.

Copy link
Member

Choose a reason for hiding this comment

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

ah yeah its just a wrapper here. This file I believe is meant to be the internal utils of the compiler decorators. If we want something exposed somewhere else we should move it out.

I think we could consider also removing the unsafe_ prefix from useStateMap/Set and move them out of expermiental now as its been working quite well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could consider also removing the unsafe_ prefix from useStateMap/Set and move them out of expermiental now as its been working quite well.

This is proposed in #5699.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:core Issues for @typespec/compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants