Skip to content

Conversation

@michael-small
Copy link
Collaborator

NGRX v19.1.0's release added withFeature, which is just withFeatureFactory but now directly built into ngrx/signals.

ngrx/platform@d87acb3

@michael-small
Copy link
Collaborator Author

@rainerhahnekamp

I realize now that when I asked you about this that I didn't ask if this would be a soft or hard deprecation. So I am going about this like a soft deprecation at the moment but can make changes to hard deprecate it if you want. In my opinion, I think a soft deprecation for perhaps at least a certain period of time or certain amount of versions and then a hard deprecation would be good.

I don't think the way I handled the doc page with the initial commit is as clean as it could be, but I wanted to ask a few questions first before over thinking things.

  1. Should withFeatureFactory still be in the sidenav?
    a. Personally, I think so for at least a couple versions or certain period of time, but with it being apparent that it is deprecated.
    b. Perhaps with some styling or copy that indicates the deprecation?
  2. How to handle the actual doc page?
    a. Should the deprecation warning in the doc page be in its own specially formatted display box or something? The way I understand things, I imagine with the docs site I could probably give it some custom display class.
    b. Should the whole doc page just be a deprecation warning and link to withFeature?

Copy link
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Quite happy with your approach! Do you think it would make sense for withFeatureFactory to directly call withFeature?

Regarding deprecation

If we can get the toolkit to support ng update in the next release, we could already start working on an automatic migration script for v20. That would also mean we don’t need to keep this feature around for too long. In fact, with a working migration, we could remove it entirely in v20.

Adding ng update support for the upcoming 19.2 release could be handled in a separate PR.


As for the documentation

If it’s possible to give the title in the side menu some kind of special marking—like a different color or a strikethrough—then yes, let’s go for it. Otherwise, we can leave it as is.

@michael-small
Copy link
Collaborator Author

Do you think it would make sense for withFeatureFactory to directly call withFeature

So the library's function would just be a wrapper for it and no real source code? Yeah that makes sense.

In that respect, I think maybe then the copy for the docs should just be pulled out and the page be a glorified link to the NgRx docs until deprecation IMO.

If it’s possible to give the title in the side menu some kind of special marking—like a different color or a strikethrough—then yes, let’s go for it. Otherwise, we can leave it as is.

I'll look into that after work

@rainerhahnekamp
Copy link
Collaborator

So the library's function would just be a wrapper for it and no real source code? Yeah that makes sense.
In that respect, I think maybe then the copy for the docs should just be pulled out and the page be a glorified link to the NgRx docs until deprecation IMO.

Yes!

@michael-small
Copy link
Collaborator Author

michael-small commented Apr 3, 2025

Update (if none of these are deal breakers then I am fine with the PR as is)

Docs:

  • Removed the link from the extensions landing page
  • I could not style the sidenav link right without some issue.
    • Trying to add a custom class by changing the metadata of the link worked for styling, but then when clicking on the link the sidenav disappeared once you navigated to the page. Happened in both the "link" and "html" type Docusaurus sidenav link types.
    • I tried adding "(decprecated)" to the name but that seemed verbose and clunky IMO. I think the page stands for itself.
  • Now the doc page is mostly the import and description but then the rest of the info about the deprecation is wrapped in a warning box.

Implementation:

  • I am not sure how to handle the wrapping of the code in a meaningful way. To preserve the typing, 90% of the code is effectively the same, but even more code has to be done to cast the signature to not include the Prettify. Upon reflection on this, it makes sense. The bulk of this util is mostly about types anyways. I may be overthinking this, but I found this to be pointless if not even more complexity than just letting it be as-is.
  • Unless there is some drift with how withFeature may be changed before the toolkit can remove withFeatureFactory, I think leaving it as-is is more effective than trying to make it be a wrapper.

…withFeature` wrapper

I explain in the PR that I find the wrapper approach more effort/complication than it is worth. Removing this for now as I am not currently changing the implementation.
@rainerhahnekamp
Copy link
Collaborator

All right, looks good to me. Would you like to add anything, or are we ready to merge?

@michael-small
Copy link
Collaborator Author

I think it's ready

@rainerhahnekamp rainerhahnekamp merged commit 0b1a4b9 into angular-architects:main Apr 3, 2025
1 check passed
@rainerhahnekamp
Copy link
Collaborator

Thanks again @michael-small

@michael-small michael-small deleted the deprecate-withFeatureFactory branch October 7, 2025 23:07
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.

2 participants