Skip to content

feat(template): make rxFor stable #1369

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Aug 28, 2022
Merged

feat(template): make rxFor stable #1369

merged 17 commits into from
Aug 28, 2022

Conversation

hoebbelsB
Copy link
Member

@hoebbelsB hoebbelsB commented Jun 9, 2022 •
edited
Loading

Description

this PR makes sure rxFor is in a state where it can be considered as stable and moved from the experimental package.
I've implemented a test suite which basically resembles the original ngFor spec.

  • move rxFor to template/
  • implement migration script
  • make sure rxFor acts as drop-in replacement for ngFor
  • implement unit tests covering the ngFor behavior
  • implement unit tests covering rxFor specific API

@nx-cloud
Copy link

nx-cloud bot commented Jun 9, 2022 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 21f2bea. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

@hoebbelsB hoebbelsB requested review from edbzn and Karnaukhov-kh June 9, 2022 21:18
@github-actions github-actions bot added </> Template @rx-angular/template related 🔬 Experimental Experimental: Feature, docs, demos 🛠️ CDK CDK related labels Jun 9, 2022
@hoebbelsB hoebbelsB requested a review from BioPhoton June 9, 2022 21:18
edbzn
edbzn reviewed Jun 10, 2022
View reviewed changes
Copy link
Member

@edbzn edbzn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you explain to me the idea of tracking by "context" where you pass the component instance reference to the trackBy fn, I'm not sure to understand the idea.

It would be nice to provide an SSR test to be sure rxFor is rendering on the server.

@hoebbelsB
Copy link
Member Author

Could you explain to me the idea of tracking by "context" where you pass the component instance reference to the trackBy fn, I'm not sure to understand the idea.

It would be nice to provide an SSR test to be sure rxFor is rendering on the server.

acutally, this is again copied from https://github.com/angular/angular/blob/main/packages/common/test/directives/ng_for_spec.ts. I just wanted to be very sure our implementation passes all the tests the original ngFor passes as well

@hoebbelsB hoebbelsB marked this pull request as ready for review June 11, 2022 01:01
niklaas
niklaas reviewed Jun 15, 2022
View reviewed changes
Copy link
Contributor

@niklaas niklaas left a comment

Choose a reason for hiding this comment

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

Nice to see this going to production! 🙌🏼

Just took a very quick look and saw the following:

@edbzn edbzn merged commit 234c6ee into main Aug 28, 2022
@edbzn edbzn deleted the test/make-rx-for-stable branch August 28, 2022 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edbzn edbzn edbzn left review comments

@Karnaukhov-kh Karnaukhov-kh Awaiting requested review from Karnaukhov-kh

@BioPhoton BioPhoton Awaiting requested review from BioPhoton

+1 more reviewer

@niklaas niklaas niklaas left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
🛠️ CDK CDK related 🔬 Experimental Experimental: Feature, docs, demos </> Template @rx-angular/template related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hoebbelsB @niklaas @edbzn @BioPhoton