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

✨ subscription shape #264

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

chengcyber
Copy link

This PR supports object subscription for values and value.

For this issue #261

@chengcyber chengcyber force-pushed the feat-subscription-shape branch from 09f8daf to 0c3b207 Compare August 8, 2019 14:01
@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #264 into master will decrease coverage by 1.62%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
- Coverage     100%   98.37%   -1.63%     
==========================================
  Files          12       12              
  Lines         541      555      +14     
  Branches      109      116       +7     
==========================================
+ Hits          541      546       +5     
- Misses          0        5       +5     
- Partials        0        4       +4
Impacted Files Coverage Δ
src/subscriptionFilter.js 57.14% <40%> (-42.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e988c49...0c3b207. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #264 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #264   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines         558    558           
  Branches      119    119           
=====================================
  Hits          558    558

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b0cf60...4572667. Read the comment docs.

@erikras
Copy link
Member

erikras commented Aug 10, 2019

Add a test showing that this works?

@chengcyber chengcyber force-pushed the feat-subscription-shape branch from a56413e to 1b0cf60 Compare August 10, 2019 15:25
@chengcyber
Copy link
Author

hi @erikras , codecov backs to 100% after adding some tests, plz take another look, thx.

@chengcyber
Copy link
Author

ping @erikras

@Dragomir-Ivanov
Copy link
Contributor

@erikras What can this be merged? I think that its valuable feature, and I could make use of it.

@erikras
Copy link
Member

erikras commented Jan 23, 2020

If I'm understanding the desire correctly, it's that you want to pass something like:

{
  values: {
    firstName: true,
    lastName: true
  }
}

...and only be notified when the values of those two fields change? Sort of like a MongoDB projection.

I've written a test that actually goes all the way through createForm() and it doesn't seem to work. Can you investigate further into why?

@chengcyber
Copy link
Author

chengcyber commented Jan 26, 2020

Hi, @erikras.
After check the projection doc and the test case, I think there might be some concept issue here. The core part of my proposal is the projection of subscription to reduce the number of subscriber be called. I would like use the following test to express my thoughts.

    form.subscribe(spy, {
      values: {
        name: true,
        email: true
      }
    })

means "I" only care about name and email. No matter what other values change won't notify this subscriber. Meanwhile the values of form data is not projected right now. if the form data contains name, email, phone, they are all passed to the component.

So, the failing test should be

    expect(spy.mock.calls[2][0]).toEqual({
      values: { name: 'erikras', email: '[email protected]', phone: '867-5309' }
    })

which should be passed

the working part is

    form.change('phone', '867-5309')
    expect(spy).toHaveBeenCalledTimes(2)

even the phone field changes, this subscriber not care about it, so no notification comes!


I'm not sure whether we should do form values projection. because it is a performance boost in my opinion. Any suggestions?

@Dragomir-Ivanov
Copy link
Contributor

Dragomir-Ivanov commented Mar 3, 2023

The way to do it with recent React, is to create a component and repeat useField("field-name"), with the fields you want to react to.

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.

3 participants