cssinjs / theming

Unified CSSinJS theming solution for React
300 stars 39 forks source link

Upgrade dependencies; support React 16 #45

Closed hburrows closed 6 years ago

hburrows commented 6 years ago

This is a minimal set of changes to upgrade all dependencies and create compatibility with React 16

There are warnings in the tests about needing to shim requestAnimationFrame and also noise from Exceptions that are purposely thrown... but all 53 tests pass. I looked into introducing a React error boundary (componentDidCatch) to catch and handle the errors but we really want them to throw back to the test harness and be handled there. So while it's a a bit noisy I think it's correct. The requestAnimationFrame warnings are annoying and I'm not finding an easy way to fix them with ava -- but it could be that I'm not that familiar with ava.

Fixes #43

hburrows commented 6 years ago

Node v4 tests are failing. Do we care about Node v4 still?

hburrows commented 6 years ago

Upgraded version to 1.2.0. Not sure if this is deserving of 2.0.0 or not. I'm guessing not because nothing changes except dependencies

vesparny commented 6 years ago

Just an heads up. brcast Unsubscription API changed in v3.

The subscribe method now returns a subscription Id you need to pass to the new method broadcast.unsubscribe()

I’m afraid now you are not unsubscribing when unmounting. This would create memory leaks

hburrows commented 6 years ago

@vesparny Thank-you !! I'm completely new to this and this repo so I really appreciate the input.

hburrows commented 6 years ago

Node v4 tests are failing because I upgraded browser-env. If node v4 support is desired then we can downgrade to browser-env@2

vesparny commented 6 years ago

No worries, I won’t probably have time to actively push code but I’ll do my best to keep an eye on notifications 😉

oliviertassinari commented 6 years ago

@hburrows Material-UI upgraded to brcast v3 some time ago, maybe you can have a look at the upgrade commit to see what's at stake?

hburrows commented 6 years ago

Updated usage of brcast to conform to version 3 api. Updated ThemeProvider unmount test to honor brcast@3 API

kof commented 6 years ago

Build is failing:

  ✖ src/create-theme-listener.test.js exited with a non-zero exit code: 1
/home/travis/build/cssinjs/theming/node_modules/browser-env/node_modules/window/src/index.js:3
const { JSDOM } = require('jsdom');
      ^
SyntaxError: Unexpected token {
kof commented 6 years ago

It tests travis has 3 node versions in the list

https://github.com/cssinjs/theming/blob/master/.travis.yml#L2

I have no idea why, we use node for building and so can use just any latest stable version

hburrows commented 6 years ago

@kof The failing build you're referring to is the failed travis build? browser-env @ >= 3 no longer supports node v4. We can downgrade tobrowser-env@2 if node v4 support is desired.... or we can remove node v4 support from travis.yml

kof commented 6 years ago

yes, lets remove node 4

hburrows commented 6 years ago

@kof Tests are now passing with node v4 support axed

kof commented 6 years ago

merged!

hburrows commented 6 years ago

@kof Are you OK if I clean-up the obsolete pull-requests from greenkeeper?

kof commented 6 years ago

please, do it!

kof commented 6 years ago

@hburrows just pulled master, tests are failing, do you have an idea why?

kof commented 6 years ago

Sorry, they are not failing, but a lot of warnings and errors, we should fix them before moving forward with the release.

hburrows commented 6 years ago

The warnings are interesting.... they're from React 16's enhancement error reporting. Errors are getting purposely thrown and caught in ava -- as expected. React is now issuing these warnings. Presumably we can catch them and handle them via componentDidCatch. We'll need to rework the tests for this. The warnings are certainly a lot of noise but technically are correct. I agree it would be best to eliminate them.

hburrows commented 6 years ago

@kof I've been experimenting on eliminating the error output from the tests when an Error is thrown. I believe this is something that enzyme is doing and I can't see anyway to eliminate it. I initially thought if I caught the Errors via componentDidCatch the error output would go away but that's not the case -- it's the same. Using componentDidCatch for these tests works fine but I don't see any advantage to re-writing them especially since it doesn't change the error output. Running the same component tree through Jest results in componentDidCatch getting properly called but no error output is generated -- this is why I think it's an enzyme thing. I welcome your thoughts. Would love to see react-jss update this dependency.