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

migration of @nivo/core to typescript and (partial) upgrade to React 18 #2046

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

tkonopka
Copy link
Contributor

Addresses #1219, #884 (partially)

Summary

  • migrated @nivo/core to typescript
  • moved @nivo/recompose from packages to deprecated.
  • merged @nivo/tooltip into @nivo/core. They had mutual peer dependencies, and I couldn't get them to build separately.
  • disabled stories and tests for non-typescript packages (affects line and waffle, also geo and parallel-coordinates). These packages are non-functioning in this branch.
  • updated dependencies to React 18.1
  • upgraded storybook to 6.5.9 to gain React 18 support
  • adjusted all packages to use the new @nivo/core and pass build/lint/test
  • further details are described in file packages/core/migration.md

Checks

  • passes make init (building all packages from scratch)
  • passes make packages-lint
  • passes make packages-test (but non-typescript tests are disabled)
  • runs make storybook and produces working charts (but non-typescript charts are disabled)

Open issues / questions

  • compatibility with website
  • storybook reports a warning about ReactDOM.render. It seems that despite having React 18 available, the pages run as if in React 17 mode. So compatibility with React 18 is unclear. (can someone help?)

@plouc, I set this as 'draft' to signal that it shouldn't be merged at this stage. But I hope some of it is useful. With your comments, we can extend this to address the issues, or move some material into smaller updates. Thanks!

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@willemmulder
Copy link

Great work!

@kacper-rogowski-wttech
Copy link

hi @plouc ,
Is there a chance, for you to take a look at this PR?

@stale stale bot added the stale label Jan 15, 2023
@willemmulder
Copy link

Bump pleeease! :-)

@stale stale bot removed the stale label Jan 16, 2023
@srowe0091
Copy link

srowe0091 commented Apr 21, 2023

storybook reports a warning about ReactDOM.render. It seems that despite having React 18 available, the pages run as if in React 17 mode. So compatibility with React 18 is unclear. (can someone help?)

@tkonopka React 18 introduces a new way to render to the dom. here is what that looks like

// Before
import { render } from 'react-dom';
const container = document.getElementById('app');
render(<App tab="home" />, container);

// After
import { createRoot } from 'react-dom/client';
const container = document.getElementById('app');
const root = createRoot(container); // createRoot(container!) if you use TypeScript
root.render(<App tab="home" />);

PS. would love to get this into React 18

@tkonopka
Copy link
Contributor Author

Hi @srowe0091. Yes, but that setup code is managed by storybook (nivo packages add components to an app that is already running). The appropriate initialization is supposed to be automatic, but here storybook seems to pick the pre-18 approach, which means the configuration or dependencies need adjustment. In a different project, I managed the switch by re-installing storybook from scratch, so that is an option.

Repository owner deleted a comment from stale bot May 8, 2023
@plouc plouc added the pinned label May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants