Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Community Feedback Request - Release a 2.0 if React removes the patents clause? #302

Open
sccolbert opened this issue Jul 16, 2017 · 35 comments

Comments

@sccolbert
Copy link
Member

One of the big reasons that Phosphor includes its own virtual DOM implementation, is because of React's patents clause. In the wake of the Apache foundation forbidding the "react+patents" license in dependencies, Facebook relicensed RocksDB as dual Apache/GPL. It's reasonable to assume that React will follow suit:
facebook/react#10191

Should that happen, what is the communities feedback on releasing a Phosphor 2.0 which does not include @phosphor/virtualdom and replaces all internal uses of that package with React?

If we do this, it will be the first external runtime dependency for Phosphor.

cc @ellisonbg @jasongrout @birdsarah @bollwyvl @jdetle @blink1073 @afshin @ian-r-rose @gnestor

@gnestor
Copy link

gnestor commented Jul 17, 2017

I think this would be a really great thing! It would save @sccolbert a ton of work and open up the project to a lot more contributors from the React community. It would also allow bundled extensions to make use of open source React components. It could also make it more straight-forward to provide and consume JupyterLab UI components (e.g. dropdown, right-click menu, toolbar, etc.).

@jdetle
Copy link
Contributor

jdetle commented Jul 17, 2017 via email

@blink1073
Copy link
Member

I also concur with Grant.

@afshin
Copy link
Member

afshin commented Jul 17, 2017

I agree with @gnestor as well

Even without the benefit broader community uptake or support, these gains are themselves nontrivial:

  • 100% inter-compatibility guarantee when functional components are exported
  • Free high-end implementation of virtual DOM whose ongoing development comes from upstream and is reliable

@jasongrout
Copy link
Member

What components use a VDOM implementation right now? The menu bar, the tab bar, and the command palette?

One thing not addressed in the above notes: of all the VDOM implementations out there, why React specifically? For one, it clearly has the largest community adoption, and presumably the greatest momentum. It does seem like we can't make the vdom implementation swappable because of the hard dependency.

Thinking of embedding one of these phosphor components in another page: Are there any problems with having React as a dependency that interfere with other uses of React on a page? Does React require being a singleton, so there would need to be some sort of deduping and possible version conflicts if React is used elsewhere on the page?

@sccolbert
Copy link
Member Author

What components use a VDOM implementation right now? The menu bar, the tab bar, and the command palette?

CommandPalette, Menu, MenuBar, and TabBar.

why React specifically

For all the reasons you mention.

Are there any problems with having React as a dependency that interfere with other uses of React on a page?

React would just need to be importable through whatever bundler you choose. We wouldn't be shipping our own copy.

@jasongrout
Copy link
Member

React would just need to be importable through whatever bundler you choose. We wouldn't be shipping our own copy.

Oh right, I forgot you aren't distributing a webpack bundle.

@gnestor
Copy link

gnestor commented Jul 17, 2017

Are there any problems with having React as a dependency that interfere with other uses of React on a page? Does React require being a singleton, so there would need to be some sort of deduping and possible version conflicts if React is used elsewhere on the page?

Yes, having more than 1 version of React loaded on a page can definitely cause issues. However, seeing as though we are using webpack to bundle everything together (including 3rd party extensions.), we should be able to dedupe and control which version of React is provided.

@ellisonbg
Copy link
Contributor

ellisonbg commented Jul 17, 2017 via email

@birdsarah
Copy link

Anything that deduplicates effort seems like a good thing.

We have been building up a collection of ui elements that we call ui-kit that all rely on VDom. We have the intention to make it public, just haven't yet. This has been a decent amount of work and if there's a chance this means that we could move to something from a larger community effort that would be great.

@ellisonbg
Copy link
Contributor

It appears that Facebook has reached a no-op decision:

https://code.facebook.com/posts/112130496157735/explaining-react-s-license

I am very disheartened by this. The most painful part is that the post appears to clarify Facebook's intent to use the patent clause as a weapon in their overall patent strategy. I would summarize their position in this way, "if you are feeling uncomfortable with the react.js patent retaliation clause, then good, cause that is why it is there."

I will be working with NumFOCUS folks this week to figure out a path forward.

@sccolbert
Copy link
Member Author

Yep. Saw that last night :(

It seems the wider community is also realizing they weren't paying close enough attention to the clause. https://news.ycombinator.com/item?id=15050841

@sccolbert
Copy link
Member Author

In light of this, I think the safest way forward right now is to not adopt React in Phosphor.

@ellisonbg
Copy link
Contributor

https://code.facebook.com/posts/300798627056246

@blink1073
Copy link
Member

Reopening in light of the license change.

@jjrv
Copy link

jjrv commented Sep 29, 2017

@phosphor/virtualdom is currently about 940 lines of code, of which maybe a half is element, attribute and property names. Doesn't sound so nice to depend on React just for that :/

For me the best things about PhosphorJS are TypeScript and that it doesn't bring in junk like lodash, jQuery, React and dozens of totally useless and incompetent left-pad style packages. How much extra work would it be to design so that React is the "official" vdom implementation, but 3rd parties could replace it?

Then there's Vue, Mithril and Moon or other bloated alternatives like Angular or Aurelia. Hopefully PhosphorJS could be impartial to such choices.

Dojo 2 will use maquette which is also all TypeScript and with a codebase about the size of the current @phosphor/virtualdom. Not saying it should be chosen instead, but if there's a suitable abstraction in place in PhosphorJS, for example I could maintain a phosphor-maquette package to replace React in my own projects.

@jdetle
Copy link
Contributor

jdetle commented Oct 2, 2017

What would it take to have phosphor widgets that are compatible with react, while still maintaining widgets that are compatible with the phosphor vdom?

@sccolbert
Copy link
Member Author

@jdetle not worth the effort.

@blink1073
Copy link
Member

@jjrv, an API compatible virtual DOM implementation (like preact-compat or a shim on top of something like maquette) could be swapped out at bundle time using something like require.alias in WebPack.

@jjrv
Copy link

jjrv commented Oct 2, 2017

@blink1073 in that case it would be nice if a subset of React API not much larger than the current vdom API was used. Any necessary shim could then be kept small. I'm sure PhosphorJS would also work great in some Angular projects and it would be silly for them to also include React.

@sccolbert
Copy link
Member Author

To be clear, we're only talking about @phosphor/widgets here. The rest of the Phosphor packages don't depend on vdom.

@sccolbert
Copy link
Member Author

sccolbert commented Oct 2, 2017

@jjrv It's also silly for me to maintain a separate vdom library at this point, given that React is more complete, has a large community, several useful third part components, and now an MIT license.

@veeramarni
Copy link

Is it suppose to work with react dom now or still using a separate vdom?

@ellisonbg
Copy link
Contributor

ellisonbg commented Jan 15, 2018 via email

@veeramarni
Copy link

Ok, do you see any problem implementing this way?

export default class ReactWidget<TProps = {}> extends Widget {
  private _cssClass = style({
    backgroundColor: appBackgroundColor,
    display: 'flex',
    flexDirection: 'column',
    minHeight: '50px',
    minWidth: '50px',
    padding: '4px'
  });

  constructor(private component: React.ComponentClass<TProps>, private props?: TProps) {
    super();
    this.addClass(this._cssClass);
  }

  protected onAfterAttach(_: Message): void {
    this.update();
  }

  protected onBeforeDetach(_: Message): void {
    ReactDOM.unmountComponentAtNode(this.node);
  }

  protected onUpdateRequest(_: Message): void {
    ReactDOM.render(
      React.createElement(AppContainer, {}, React.createElement(this.component, this.props)),
      this.node
    );
  }
}

@ellisonbg
Copy link
Contributor

ellisonbg commented Jan 15, 2018 via email

@lukehutch
Copy link

Why not use Inferno instead? Its it is smaller and much faster than React, and in particular its virtual DOM diff implementation is much faster than React's.

https://github.com/infernojs/inferno

@bionicles
Copy link

Hi, How can we make a react-phosphor / react-jupyterlab type thing so we can embed JupyterLab in our React app and share state? Typescript noob but studying it now

@gnestor
Copy link

gnestor commented May 18, 2019

@saulshanabrook's response on Gitter:

One short answer: You can't. Not only is JupyterLab not built with React it doesn't have global state storage.

Longer answer: You could wrap JupyterLab in a React component, but you would have to manually link the state to any changes in JupyterLab's application. We have been transitioning some of JupyterLab to use React internally and if you are interested in solving this problem long term, we would love to have your help with that. If that is something you are interested in, I am happy to help get you started there.

Which parts of JupyterLab are you looking to control externally?

@bionicles
Copy link

@gnestor

  1. We need to be able to control the location / dimensions of the lab, so it can fit inside a desired position on the app (Potentially options.hostId passed to jupyterlab can control this)

  2. We need to pass maps of nodes / links / selections to jupyterlab kernel, so user can prototype graph algorithms quickly. When the kernel updates the graph, it could dispatch CRUD operations to the parent react component, and when the user updates graph data in the app, it could pass this data as a prop to the jupyterlab

like

then inside jupyterlab component we could just render the lab one time, and watch for changes to the graph data, and dispatch updates to react state

@gnestor
Copy link

gnestor commented May 20, 2019

This all sounds possible but I can't say for sure without seeing some example code.

We need to be able to control the location / dimensions of the lab, so it can fit inside a desired position on the app (Potentially options.hostId passed to jupyterlab can control this)

I'm not familiar with options.hostId but you can certainly put JupyterLab in an iframe and control the dimensions of that.

We need to pass maps of nodes / links / selections to jupyterlab kernel, so user can prototype graph algorithms quickly. When the kernel updates the graph, it could dispatch CRUD operations to the parent react component, and when the user updates graph data in the app, it could pass this data as a prop to the jupyterlab

You can create a labextension that hooks into kernels and respond to kernel events, messages, etc. and communicate with the parent React app from there.

@bionicles
Copy link

bionicles commented May 20, 2019

here's a repo where i've attempted to create a react app for jupyterlab

https://github.com/bionicles/lab-component

I added index.js of examples/app to a "useEffect" hook inside a boilerplate create-react-app

"yarn start" and it tries to work but cannot find a kernel server and has some bugs:
the jupyterlab stuff automatically fills the whole screen when we want it to just fill the parent div
Unexpected token < in JSON at position 0
TypeError: Cannot read property '0' of undefined

next I tried adding main.py also from examples/app -- but python main.py fails as there is no template.html

how do you make the main.py function play nice with react app instead of html template?

EDIT: added index.html and webpack.config.js however when python main.py launches index.html it seeks bundle.js, which does not exist because create-react-app has conflicting build settings

@jasongrout
Copy link
Member

For reference, @bionicles created jupyterlab/jupyterlab#6380 to continue the conversation about embedding in React.

@blink1073
Copy link
Member

blink1073 commented Oct 9, 2019

@jasongrout recently brought up the point that by depending on React in @phosphor/widgets, we would have to bump the major version of our library when React has a major version bump. The TypeScript definitions for React have also been historically brittle. In JupyterLab, we have been forced to declare the types as a pinned dependency.

I think instead we should make it easier to use React with Phosphor.

  • Add the RenderWidget
  • Utilize the fact that jsx can now be declared per module in TypeScript
  • Add a pass through for React renderers to the Phosphor vdom
  • Add examples of using React in a Phosphor widget and vice versa.
  • Investigate allowing React components to be used as the renderers for the core Phosphor widgets.

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

No branches or pull requests