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

Use React for TSX #3133

Merged
merged 14 commits into from Oct 26, 2017
Merged

Use React for TSX #3133

merged 14 commits into from Oct 26, 2017

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 23, 2017

This is the first step to converting to React as our Virtual DOM implementation, and is a pre-requisite to absorbing the VDOM and JSON tree renderers from https://github.com/jupyterlab/jupyter-renderers.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Oct 23, 2017

<div className="jp-MoreHorizIcon" />
</div>
]
</div>
Copy link
Contributor

@sccolbert sccolbert Oct 23, 2017

React 0.16 allows you to use arrays IIRC

Copy link
Member Author

@blink1073 blink1073 Oct 23, 2017

The Vega Voyager and JSON tree components depend on React 15. I was deferring the decision about whether to allow multiple versions or force there to be a React 16.

Copy link
Contributor

@sccolbert sccolbert Oct 23, 2017

Since this changes the DOM structure, is a CSS update also required?

Copy link
Contributor

@sccolbert sccolbert Oct 23, 2017

It also seems odd for us to constrain our React version based on plugins, and not the other way around...

Copy link
Member Author

@blink1073 blink1073 Oct 23, 2017

Okay, for now I'll have us depend on React 16 and not enforce it globally, and we can decide later whether to force there to be only one React.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 23, 2017

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Oct 23, 2017

For JSON tree, it looks like we'd get console warnings if we forced React 16: alexkuz/react-json-tree#94. Voyager depends on a dozen or so other React-based libraries, so I suspect it would have similar problems.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Oct 23, 2017

Do we know if it's possible for both 0.15 and 0.16 to exist on the page?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Oct 23, 2017

I'll check right now.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Oct 23, 2017

Added the json renderer using addsibling, checked for both versions of react in the bundle. Everything loads fine with no console errors:

image

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Oct 23, 2017

Saw this though when opening the launcher:

image

@blink1073 blink1073 changed the title Use React for TSX [WIP] Use React for TSX Oct 23, 2017
@gnestor
Copy link
Contributor

@gnestor gnestor commented Oct 23, 2017

Excellent!! I'm going to test this myself. I'm getting the following TS compile error: ../json-extension/src/component.tsx(68,13): error TS2322: Type 'Timer' is not assignable to type 'number'. If I switch my target to ES6, then no prob. Are we allowed to target ES6?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Oct 23, 2017

I had to compile the json extension separately, since we can only have one version of @types/react in all-packages.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Oct 23, 2017

@gnestor Something is bringing in the nodejs typings which redefines the return type of setTimeout.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Oct 23, 2017

We still need to target ES5.

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Oct 23, 2017

That warning will come up even in old versions of React. It's wise to use unique keys with arrays of elements (not necessarily the index of the item). React tries to let you get going quickly, which is why it allows you to go without keys. Good to fix before production though. 😄

@gnestor
Copy link
Contributor

@gnestor gnestor commented Oct 23, 2017

@blink1073 The error is caused by React rendering an array of elements. Each element in the array should include a unique key property, so you can usually do something like myArray.map((item, index) => <span key={index}>{item}</span>.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Oct 23, 2017

Got it, thanks!

@blink1073 blink1073 closed this Oct 23, 2017
@blink1073 blink1073 reopened this Oct 23, 2017
@blink1073 blink1073 changed the title [WIP] Use React for TSX Use React for TSX Oct 23, 2017
@@ -303,14 +303,14 @@ class Launcher extends VDomRenderer<LauncherModel> {
let kernel = KERNEL_CATEGORIES.indexOf(cat) > -1;
if (cat in categories) {
section = (
<div className='jp-Launcher-section'>
<div className='jp-Launcher-section' key={index}>
Copy link
Member

@rgbkrk rgbkrk Oct 23, 2017

If the category is known and unique, it's a better key than the index. This matters if the ordering of the elements could change or if any are added or deleted.

Copy link
Member Author

@blink1073 blink1073 Oct 23, 2017

Updated, thanks. It looks like we can't do the same for the Cards because they could differ only by callback.

Copy link
Member Author

@blink1073 blink1073 Oct 23, 2017

I stand corrected, we can and should enforce uniqueness using a stable JSON.stringify.

@blink1073 blink1073 force-pushed the add-vdom-renderer branch from 9a613ec to 1934a33 Oct 26, 2017
afshin
afshin approved these changes Oct 26, 2017
Copy link
Member

@afshin afshin left a comment

👍

@afshin afshin merged commit ee16a13 into jupyterlab:master Oct 26, 2017
2 checks passed
@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Oct 26, 2017

😄

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 27, 2017

@blink1073 blink1073 mentioned this pull request Nov 8, 2017
@blink1073 blink1073 deleted the add-vdom-renderer branch Nov 23, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants