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

Remove unstable_read() in favor of direct dispatcher call #13861

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 16, 2018

It's an advanced feature that shouldn't be used by application code directly. For example classes are supposed to use contextType (reactjs/react.dev#1265) instead.

So we want to hide it a bit deeper. This makes it only available via direct dispatcher read. Also removes an extra runtime check.

Libraries can still use it by calling the dispatcher directly.

@sizebot
Copy link

sizebot commented Oct 16, 2018

React: size: -1.2%, gzip: -0.6%

Details of bundled changes.

Comparing: 21a79a1...35cf54c

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js -0.9% -0.7% 95.03 KB 94.15 KB 24.94 KB 24.77 KB UMD_DEV
react.production.min.js -1.2% -0.6% 11.86 KB 11.72 KB 4.66 KB 4.64 KB UMD_PROD
react.development.js -1.5% -1.2% 57.74 KB 56.86 KB 15.47 KB 15.28 KB NODE_DEV
react.production.min.js -2.3% -1.7% 6.28 KB 6.14 KB 2.66 KB 2.62 KB NODE_PROD
React-dev.js -1.8% -1.5% 54.21 KB 53.25 KB 14.67 KB 14.46 KB FB_WWW_DEV
React-prod.js -2.2% -1.8% 14.35 KB 14.03 KB 3.93 KB 3.85 KB FB_WWW_PROD
React-profiling.js -2.2% -1.8% 14.35 KB 14.03 KB 3.93 KB 3.85 KB FB_WWW_PROFILING
react.profiling.min.js -1.0% -0.7% 14.02 KB 13.88 KB 5.18 KB 5.15 KB UMD_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@TrySound
Copy link
Contributor

This is sad. .read() api is quite useful for translations. Will you go with this at least in react-cache or we should wait for adopt syntax solution?

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 16, 2018

I mean, you can still use it in a library. Your translation library could literally do the same thing that react-cache would do.

  const ReactCurrentOwner = React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentOwner;

  function readContext(Context) {
    const dispatcher = ReactCurrentOwner.currentDispatcher;
    return dispatcher.readContext(Context);
  }

@gaearon gaearon merged commit 7685b55 into facebook:master Oct 16, 2018
@gaearon gaearon deleted the no-unstable-read branch October 16, 2018 18:58
@TrySound
Copy link
Contributor

Great! Thank you.

@markerikson
Copy link
Contributor

Uh... that seems really icky. Why are you effectively encouraging digging around in React's internals? What's the reasoning for removing this?

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Oct 17, 2018

unstable_ was as equally going to break as the other options so it wasn't safer. Don't want to get stuck with an API that scales with the number of context in the API. We don't really do virtual method APIs other than setState. We need to communicate that this is really not a designed API ready for use and the only user of it (react-cache) also isn't released (in fact we even removed its usage in react-cache).

linjiajian999 pushed a commit to linjiajian999/react that referenced this pull request Oct 22, 2018
…3861)

* Remove unstable_read() in favor of direct dispatcher call

* This no longer throws immediately
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
…3861)

* Remove unstable_read() in favor of direct dispatcher call

* This no longer throws immediately
@markerikson
Copy link
Contributor

So I'm starting to poke around at potential ways to re-revamp connect to use direct subscriptions again (per reduxjs/react-redux#1177 ), and the very first question I have is, "how are we supposed to get the store reference out of context so we can subscribe in cDM?"

Context.unstable_read() still seems like it would have been the best answer to that question.

@Jessidhia
Copy link
Contributor

Or useContext, which is just a hook version of readContext. But that requires 16.8...

@gaearon
Copy link
Collaborator Author

gaearon commented Feb 4, 2019

contextType in a class (16.6+), useContext in a function (coming).

Don’t have a good answer for you that’s guaranteed to work earlier.

@markerikson
Copy link
Contributor

Yeah, I know. That's why I'm diving off the deep end and experimenting with a hooks-based approach first.

contextType isn't exactly an option atm, because in v6 we allow the user to customize what context the component uses by passing it as a prop.

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

Successfully merging this pull request may close these issues.

7 participants