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

Deprecate using tracked in classic classes #728

Closed
wants to merge 2 commits into from

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Mar 18, 2021

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

This seems like a good thing to deprecate, but I'd rather see the deprecation be until 5.0 not 4.0.

@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 19, 2021

@rwjblue waiting until 5.0 would lock us into having to provide a shim around @glimmer/tracking until then, which I would prefer not to. Ideally, @glimmer/tracking could be completely independent of Ember sooner rather than later.

Does the alternative of exposing trackedClassic() from @ember/object/compat work as a short term solution? Effectively provide an easy-to-codemod alias for the existing functionality?

@chancancode
Copy link
Member

Is this really unused? I seem to remember us using it, though I would have to check to be really sure.

It also seems somewhat arbitrary to fall off a cliff on this particular feature and this feature only, I think we got a lot of millage out of the flexibility and simplicity of the decoupled migration model. It also breaks the general expectations about decorators in the framework (on this particular decorator only).

If it’s just about not loosing out on a good naming, this seems like too heavy-handed to me. Certainly there would be other just as good options like new tracked(...) (detect new.target) or just make it a different spelling (like Tracked to signify that it is a constructor).

I am also not sure if it’s actually a fundamentally good idea to overload this to mean two pretty different things. We did it with {{action}} and that’s kind of a mixed bag outcome (probably mostly bad). It would be somewhat harder to ask questions/google/lookup documentations, and things like typescript/vscode would have a somewhat less ideal UX.

@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 19, 2021

I am also not sure if it’s actually a fundamentally good idea to overload this to mean two pretty different things. We did it with {{action}} and that’s kind of a mixed bag outcome (probably mostly bad). It would be somewhat harder to ask questions/google/lookup documentations, and things like typescript/vscode would have a somewhat less ideal UX.

@chancancode I believe this is an orthogonal point of discussion. Whether or not we decide to use tracked({}) or tracked([]), it would be useful to have the option to, which is partially what this RFC is about enabling.

@rwjblue
Copy link
Member

rwjblue commented Mar 19, 2021

@rwjblue waiting until 5.0 would lock us into having to provide a shim around @glimmer/tracking until then, which I would prefer not to. Ideally, @glimmer/tracking could be completely independent of Ember sooner rather than later.

I don't think this is correct? The definition of what the "invoke" does here (e.g. non-decorator use case) could absolutely be provided by @glimmer/global-context or any number of other things to avoid the coupling.

@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 19, 2021

That's a good point. It would be unfortunate to have to tie Ember in that way I think, specifically for a to-be-deprecated feature (classic classes), but it would work to solve that issue.

@boris-petrov
Copy link

boris-petrov commented Mar 22, 2021

In order to use tracked properties, you must convert to using a native class.

Does that mean that this will work:

import Component from "@ember/component";

class ClassicComponent extends Component {
  @tracked foo;
}

Or by native class you mean non-classic Ember stuff? If so, that should be re-worded.

@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 22, 2021

@boris-petrov that would work! This would only deprecating using the classic syntax, any classic classes that work as native classes would continue to work with @tracked

@sandstrom
Copy link
Contributor

Export a new value, trackedClassic, from @ember/object/compat. This would only be usable as a classic decorator in classic classes, and the current form of tracked() would still be deprecated.

Sounds like a good idea!

My gut feeling is that this is indeed rarely used, so the churn cost should be pretty low. Especially if trackedClassic would be around.

Speaking for our app, we've still got a good deal of classic components. We're gradually migrating, when we have other reasons to refactor code we'll rewrite with glimmer plus angle brackets and tracked.

Just my view though, others may have another perspective.

@runspired
Copy link
Contributor

runspired commented Apr 10, 2021

The classic syntax is indeed heavily used by many apps I've encountered, most commonly when migrating a very large app when the native class codemod cannot yet be run. It seems to be a transient middle state although a very useful one: the native class codemod doesn't take a lot of apps very far until they've cleaned up certain other patterns that can take some time, and many codemods were written to only mod classic syntax to better classic syntax patterns prior to moving to native classes so these teams wait to run the native class codemod until they've completed this other work.

I don't know that i agree with overloading the signature but from the perspective of whether the classic syntax is used think the deprecation until should be the same as for classic syntax as many of these mid-migration apps are on latest Ember versions. Folks don't always upgrade right away and it would suck for there to be a mismatch here where they needed to downgrade or pin at an ember version because classic still worked but tracked on classic didn't.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

Well this was not part of 4.0. Is the only thing blocking the version we do it in?

@runspired
Copy link
Contributor

@wagenet I think so

@wagenet
Copy link
Member

wagenet commented Jul 29, 2022

@chriskrycho would you think this falls under #832 or is it separate?

@chriskrycho
Copy link
Contributor

Roughly: classic classes themselves will go away. We could remove this earlier than those, though, so please do include it in that list!

@wagenet
Copy link
Member

wagenet commented Jul 30, 2022

I’ll add this to the #832 list but also leave it open for now.

@chriskrycho
Copy link
Contributor

This fell off my radar until I was doing a triage pass, but it pairs nicely with #876; I'm going to bring it to Framework this week. For good or for ill, it looks like it's going to end up targeting 6.0, since we are now in the 4.10 beta cycle and we freeze deprecations at the .10 minor release.

@chriskrycho chriskrycho added the S-Exploring In the Exploring RFC Stage label Dec 1, 2022
@chriskrycho
Copy link
Contributor

We talked about this today at Framework, and rather than pushing this direction (and deprecating things piecemeal) we are going to FCP to close—by getting rid of it holistically as part of what we're discussing around Classic on #832.

@chriskrycho chriskrycho added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Dec 2, 2022
@wagenet wagenet closed this Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed. S-Exploring In the Exploring RFC Stage T-deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants