-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGFIX BETA] Make the *type* for SafeString
public
#20373
Conversation
SafeString
publicSafeString
public
d9a4cb9
to
3a103b1
Compare
This type has been effectively "intimate" for many years, and fits in the same bucket as e.g. `Transition`: it is not user-constructible, but will be constructed by the framework and users need to be able to name it. For example, `ember-intl` needs to be able to see that a string has been marked as trusted to do the right thing to emit it. Internally, clean up a few long-standing TS issues (`any` etc.), make `SafeString` explicitly implement the contract from Glimmer so that if that contract changes, we will know at the definition site, and make the implementation details of how `SafeString` handles the string it wraps `private`. (This does not use a `#`-private field because private class fields have some non-trivial overhead in transpiled contexts, and `SafeString` can appear in fairly hot rendering paths.)
3a103b1
to
00c69d2
Compare
SafeString
publicSafeString
public
SafeString
publicSafeString
public
public string: string; | ||
import type { SafeString as GlimmerSafeString } from '@glimmer/runtime'; | ||
|
||
export class SafeString implements GlimmerSafeString { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized after landing this: we'll need docs here to make it show up appropriately. I'll follow up to that effect tomorrow.
I'm not sure where this falls on the 'is a bug / is a mistake in our code' spectrum, but we were accessing the results of |
The class itself was not public, so relying on any part of it was subject to breakage at any time. The field was only |
I hear that, and honestly this is no big deal, but I wasn't relying on anything like a type when we wrote this code 8 years ago 😁, what we were using was the result of |
This type has been effectively "intimate" for many years, and fits in the same bucket as e.g.
Transition
: it is not user-constructible, but will be constructed by the framework and users need to be able to name it. For example,ember-intl
needs to be able to see that a string has been marked as trusted to do the right thing to emit it.Internally, clean up a few long-standing TS issues (
any
etc.), makeSafeString
explicitly implement the contract from Glimmer so that if that contract changes, we will know at the definition site, and make the implementation details of howSafeString
handles the string it wrapsprivate
. (This does not use a#
-private field because private class fields have some non-trivial overhead in transpiled contexts, andSafeString
can appear in fairly hot rendering paths.)I will back port this to the preview types as well once we merge this.