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

Chore: Refactor to the new Context.unstable_read() API #814

Closed
jaredpalmer opened this issue Aug 7, 2018 · 4 comments
Closed

Chore: Refactor to the new Context.unstable_read() API #814

jaredpalmer opened this issue Aug 7, 2018 · 4 comments
Assignees

Comments

@jaredpalmer
Copy link
Owner

Current Behavior

Currently, we use connect() to wire up components to Formik's context. This creates an extra node for <Field>, <Form>, and <FastField> because connect() is simply a wrapper around the <FormikContext.Consumer> render prop.

Desired Behavior

Remove connect() usage internally and instead use Andrew's forthcoming Context.unstable_read() (facebook/react#13139) which allows reading context within any render-phase function.

Suggested Solution

Pretty straight forward. We rename <XXXXImpl> to just <XXXX> and get rid of connect() usage internally (i.e. remove my poorly typed HoC TypeScript nonsense at the bottom of the files). We will still need interface FormikContext because I expect that the TS type signature for .unstable_read() will take a TS generic.

// FormikContext.js
export const FormikContext = React.createContext('');
// Field.js
import React from 'react'
import { FormikContext } from './FormikContext';

export const Field = ({ component = 'input', name, ...rest }) => {
  // Yes. This is real.
  const { handleChange, handleBlur, values } = FormikContext.unstable_read();
  
  // handwaiving the rest...
  return React.createElement(component, {
    onChange: handleChange,
    onBlur: handleBlur,
    value: values[name] || '',
    name,
    ...rest,
  });
};

Who does this impact? Who is this for?

We will still export connect() so no breaking changes there. However, enzyme tests may break AGAIN because of these changes because we are going to be removing a node from the tree for each component (no more FieldImpl, just Field). 🤣 🤣. We can now go back to shallow rendering instead of mount though!

Additional context / Discussion

  • We may end up moving <Field> to an SFC and moving all the class functions down into render. I need to ask Dan/Andrew about pros / cons.
  • Decide if we should deprecate connect() with a warning, since this API will be the future.
  • Would be great to expose new "context getters" ™️ so that people don't even need <Field> to create their own custom components? I'm not sure where these would get defined? Maybe in getDerivedStateFromProps in Formik? idk. Need to ask.
@jaredpalmer jaredpalmer self-assigned this Aug 7, 2018
@prichodko
Copy link
Collaborator

Woah, didn't know about this API 😄That's awesome. Syntactically it reminds me of this babel plugin - https://github.com/Andarist/babel-plugin-jsx-adopt.

Idea of creating "context getters" seems really cool (deletes unnecessary wrappers).

import { fieldContext } from 'formik'

const MyField = props => {
   const { value, error, touched } = fieldContext() // returning field bag
   return ( /* jsx */)
}

@jaredpalmer
Copy link
Owner Author

jaredpalmer commented Aug 10, 2018

Unfortunately .read() and .set() only works in render phase functions right now. Womp. Womp.

@jaredpalmer jaredpalmer mentioned this issue Aug 15, 2018
9 tasks
@stale
Copy link

stale bot commented Oct 9, 2018

Hola! So here's the deal, between open source and my day job and life and what not, I have a lot to manage, so I use a GitHub bot to automate a few things here and there. This particular GitHub bot is going to mark this as stale because it has not had recent activity for a while. It will be closed if no further activity occurs in a few days. Do not take this personally--seriously--this is a completely automated action. If this is a mistake, just make a comment, DM me, send a carrier pidgeon, or a smoke signal.

@stale stale bot added the stale label Oct 9, 2018
@jaredpalmer
Copy link
Owner Author

Closing. This will not be final API. Stay tuned for the real thing near React Conf in a few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants