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

[addons-react] Following React 16.3+ new Context API ? #584

Open
fsubal opened this issue Apr 6, 2018 · 6 comments
Open

[addons-react] Following React 16.3+ new Context API ? #584

fsubal opened this issue Apr 6, 2018 · 6 comments

Comments

@fsubal
Copy link

@fsubal fsubal commented Apr 6, 2018

Expected behavior

React 16.3+ has now new Context API and it is deprecating legacy one like this.context = ... or static contextTypes. Maybe functions like executeAction and getStore should be provided with the new way.

Actual behavior

fluxible-addons-react does not provide those based on new context API. It still depends on the legacy one.

Information about the Issue

https://reactjs.org/docs/context.html

Steps to reproduce the behavior

None.

@redonkulus

This comment has been minimized.

Copy link
Contributor

@redonkulus redonkulus commented Apr 16, 2018

@fsubal We will have to take a look at the new API and see how that changes the implementation. I believe React is keeping the existing context API for sometime, but that might be just until v17 is released.

redonkulus added a commit that referenced this issue Sep 27, 2018
## The devDependency [babel-eslint](https://github.com/babel/babel-eslint) was updated from `9.0.0` to `10.0.0`.
This version is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

---

<details>
<summary>Release Notes for v10.0.0</summary>

<h1>v10.0.0</h1>
<p>Small breaking change: add a peerDependency starting from the ESLint version that added a parser feature that we were monkeypatching before (and drop that code). If already using ESLint 5 shouldn't be any different.</p>
<ul>
<li>Bugfix for <code>TypeAlias</code>: <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="290308427" data-permission-text="Issue title is private" data-url="babel/babel-eslint#584" href="https://urls.greenkeeper.io/babel/babel-eslint/pull/584">#584</a></li>
</ul>
<div class="highlight highlight-source-js"><pre><span class="pl-c"><span class="pl-c">/*</span> @flow <span class="pl-c">*/</span></span>
type <span class="pl-c1">Node</span><span class="pl-k">&lt;</span><span class="pl-c1">T</span><span class="pl-k">&gt;</span> <span class="pl-k">=</span> { head<span class="pl-k">:</span> <span class="pl-c1">T</span>; tail<span class="pl-k">:</span> <span class="pl-c1">Node</span><span class="pl-k">&lt;</span><span class="pl-c1">T</span><span class="pl-k">&gt;</span> }

<span class="pl-c"><span class="pl-c">//</span> or </span>

type <span class="pl-c1">File</span> <span class="pl-k">=</span> {chunks<span class="pl-k">:</span> <span class="pl-c1">Array</span><span class="pl-k">&lt;</span>Chunk<span class="pl-k">&gt;</span>}
type Chunk <span class="pl-k">=</span> {file<span class="pl-k">:</span> <span class="pl-c1">File</span>}</pre></div>
<ul>
<li>Update to test against ESLint 5, add a peerDependency: <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="363695123" data-permission-text="Issue title is private" data-url="babel/babel-eslint#689" href="https://urls.greenkeeper.io/babel/babel-eslint/pull/689">#689</a></li>
<li>Drop monkeypatching behavior: <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="363706472" data-permission-text="Issue title is private" data-url="babel/babel-eslint#690" href="https://urls.greenkeeper.io/babel/babel-eslint/pull/690">#690</a></li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 5 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/babel/babel-eslint/commit/8f78e280a22def1128cd847b73fd7f221a047ed2"><code>8f78e28</code></a> <code>10.0.0</code></li>
<li><a href="https://urls.greenkeeper.io/babel/babel-eslint/commit/717fba7f5605f4dc1cc5531a7c24d5c9ab37a8a2"><code>717fba7</code></a> <code>test value should be switched</code></li>
<li><a href="https://urls.greenkeeper.io/babel/babel-eslint/commit/020d012c554913fea137f4129798ce31a4896dfe"><code>020d012</code></a> <code>Treat type alias declarationlike function declaration (#584)</code></li>
<li><a href="https://urls.greenkeeper.io/babel/babel-eslint/commit/b400cb1b38aaa8999d2a43f381f3ee861469ab98"><code>b400cb1</code></a> <code>Test eslint5, update peerDep (#690)</code></li>
<li><a href="https://urls.greenkeeper.io/babel/babel-eslint/commit/c333bd64cb014cac8b56062421e9a5c0d28d798b"><code>c333bd6</code></a> <code>Drop old monkeypatching behavior (#689)</code></li>
</ul>
<p>See the <a href="https://urls.greenkeeper.io/babel/babel-eslint/compare/6aa8b6f02ff83cfb14eae2432f226f113929a50f...8f78e280a22def1128cd847b73fd7f221a047ed2">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
@pwmckenna

This comment has been minimized.

Copy link
Contributor

@pwmckenna pwmckenna commented Nov 19, 2018

I don't believe it will be possible to upgrade to the new context api without a breaking change. We'll need some globally shared context object (react.createContext()) that both fluxible-addons-react can use for connectToStores etc, but that object must also be assessible to your app when injecting the context. If you could guarantee that everyone was already using exclusively provideContext to inject the fluxible context this would be possible, but we don't really have that guarantee.

I think that one of the large drivers of this upgrade will be hooks. useContext uses exclusively the new api, so if you want to use useContext you'll have to define your own new context object, and use the provider to inject that in addition to whatever you're doing currently to get context down to your connectToStores etc. That doesn't like a terrible interm solution in my mind.

The one thing I believe we should do immediately to help future proof fluxible-addons-react would be to actually expose the new fluxible context (along with some docs on how to inject it along side your existing context), even if its not using it internally. This would allow fluxible based libraries to upgrade to the latest context api, without introducing a doomed from the start context object that will never be used by connectToStores. It would make the future upgrade path much smoother, allowing you to opt into the double fluxible injection at your leisure, then once fluxible-addons-react was upgraded, there would be nothing to do even if it was a "breaking" change.

The PR will be tiny, so I'll go ahead and just create it, regardless of whether folks think this is a good idea.

@pwmckenna

This comment has been minimized.

Copy link
Contributor

@pwmckenna pwmckenna commented Nov 19, 2018

actually, as I create the PR, it occurs to me that this might be a lot cleaner if it was just a separate yahoo npm module so it can depend specifically on 16.3. the alternative is to not export it as part of the module (so you have to import from fluxible-addons-react/context or something) and have the file assert the version of react. Something like this: https://github.com/mui-org/material-ui/blob/7d6ac7f0335252d66d97e5b812a6b7d93581b833/packages/material-ui/src/ButtonBase/ButtonBase.js#L52

@pwmckenna

This comment has been minimized.

Copy link
Contributor

@pwmckenna pwmckenna commented Nov 23, 2018

in the meantime i've created fluxible-context and published it. I needed it to build the hook alternative to connectToStores, useStores as well as an internal library, but i'd be happy to move it into the yahoo org at any point.

@TasukuUno

This comment has been minimized.

Copy link

@TasukuUno TasukuUno commented Jul 4, 2019

Hi, I am one of the biggest fan of fluxible!
I have also tried to rewrite addons-react with new Context API and hooks API.
https://github.com/TasukuUno/fluxible-addons-react-16

It was not so difficult, but a new problem has occurred. handleRoute and handleHistory API of fluxible-router also depend on Legacy Context API. I think that these functions to connect component and router should be included in addons-react.

Anyway, I hope it will be supported in the near future! 😃

@johannessjoberg

This comment has been minimized.

Copy link

@johannessjoberg johannessjoberg commented Aug 23, 2019

Hi! Thanks for a great lib, been using it for years!

Any plans to go further with this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.