-
-
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
[DEPRECATION] deprecate Component#isVisible
#17948
Conversation
30e73fa
to
0de9dec
Compare
isVisible
deprecation warningisVisible
deprecation warning
9f9300e
to
7f35156
Compare
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.
Looking good! I left some minor inline comments for tweaks, but we also need to flag this for svelting.
The rough steps for svelting are:
- Add a new export from
packages/@ember/deprecated-features/index.js
, the string value should be3.11.0-beta.1
(which will be the first tagged release including the deprecation) - Import that boolean flag and surround any code supporting
isVisible
with a conditional (e.g.if (EMBER_COMPONENT_IS_VISIBLE) { /* code here */ }
)
Some example PRs adding svelting as part of a deprecation:
- [DEPRECATION] Add deprecation for Function.prototype extensions. #17910
- [DEPRECATE] Deprecates aliasMethod #17540
- Wrap all existing deprecated features for svelting. #16685
The goal here is that any code that is supporting a deprecated feature will (eventually) be able to be stripped from apps that declare that they are "deprecation free" for a given version.
Oh, also noticed, we should update the docs for |
@GavinJoyce - Sorry for being spammy here, I updated the PR description with a checklist... |
isVisible
deprecation warningisVisible
deprecation warning
7f35156
to
db90154
Compare
4f5fda9
to
d8242f9
Compare
isVisible
deprecation warningisVisible
isVisible
Component#isVisible
d8242f9
to
813ac56
Compare
packages/@ember/-internals/glimmer/lib/component-managers/curly.ts
Outdated
Show resolved
Hide resolved
81d787c
to
5f78612
Compare
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.
Looks really good, only one more small nit and I think we are good (once the deprecation guide lands).
// // think it should be a Reference<string|null>. | ||
// operations.addDynamicAttribute(element, 'style', ref as any as Reference<string>, false); | ||
}, | ||
export let IsVisibleBinding: any; |
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.
any
is a bit unfortunate here, can we make it:
export let IsVisibleBinding: undefined | {
install(element: Simple.Element, component: Component, operations: ElementOperations): void;
};
(I think mapStyleValue
is only used internally?)
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.
Thanks, I've updated it to:
export let IsVisibleBinding: undefined | {
install(element: Simple.Element, component: Component, operations: ElementOperations): void;
mapStyleValue(isVisible: boolean, component: Component): SafeString | null;
};
With my currently limited knowledge of TS, I couldn't find a way to mark mapStyleValue
as private.
977c92b
to
9960335
Compare
@rwjblue thanks for all the feedback, that will help me a lot in future deprecations. This and ember-learn/deprecation-app#451 are ready I believe |
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.
Two very minor nit-pick's, but otherwise this is good to go...
packages/@ember/-internals/glimmer/lib/component-managers/curly.ts
Outdated
Show resolved
Hide resolved
packages/@ember/-internals/glimmer/lib/component-managers/curly.ts
Outdated
Show resolved
Hide resolved
9960335
to
c27890d
Compare
Sorry @GavinJoyce, looks like a prettier error in CI now:
|
c27890d
to
0fca536
Compare
sorry - updated, thanks |
part of #17918
TODO:
toString
isVisible
as@deprecated
Component#isVisible
ember-learn/deprecation-app#451