Skip to content

✨ subscription shape#264

Open
chengcyber wants to merge 5 commits into
final-form:mainfrom
chengcyber:feat-subscription-shape
Open

✨ subscription shape#264
chengcyber wants to merge 5 commits into
final-form:mainfrom
chengcyber:feat-subscription-shape

Conversation

@chengcyber

Copy link
Copy Markdown

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

codecov Bot commented Aug 8, 2019

Copy link
Copy Markdown

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

codecov Bot commented Aug 8, 2019

Copy link
Copy Markdown

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

erikras commented Aug 10, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
Author

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

@chengcyber

Copy link
Copy Markdown
Author

ping @erikras

@Dragomir-Ivanov

Copy link
Copy Markdown
Contributor

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

@erikras

erikras commented Jan 23, 2020

Copy link
Copy Markdown
Member

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

chengcyber commented Jan 26, 2020

Copy link
Copy Markdown
Author

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: 'erikras@final-form.org', 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

Dragomir-Ivanov commented Mar 3, 2023

Copy link
Copy Markdown
Contributor

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