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

Update dependencies to support react@16.0.0 #43

Closed
damianobarbati opened this issue Sep 28, 2017 · 21 comments · Fixed by #45
Closed

Update dependencies to support react@16.0.0 #43

damianobarbati opened this issue Sep 28, 2017 · 21 comments · Fixed by #45

Comments

@damianobarbati
Copy link

As title says! :)

@kof
Copy link
Member

kof commented Oct 5, 2017

I think we should not have react in dependencies at all, it should be peerDependencies

@kof
Copy link
Member

kof commented Oct 5, 2017

@iamstarkov do you know why we use it in dependencies?

@hburrows
Copy link
Collaborator

hburrows commented Oct 6, 2017

ThemeProvider extends React.Component... otherwise React is only needed for the tests. So... I think it's needed as a dependency.

I created a branch and updated the dependencies to the latest version and updated enzyme to use its adapter (enzyme-adapter-react-16). All the tests are passing. The only issues are:

  • some tests throw Errors and no error boundaries are defined with componentDidCatch. I think this might be fine but worthy of discussion.
  • React 16 requires requestAnimationFrame which results in a warning. This is easy to shim but I can't figure out how to get ava to honor a global test setup like you can do with jest. This warning is harmless (because this package doesn't depend on raf) but it might be something that is desired to cleanup.

@kof
Copy link
Member

kof commented Oct 6, 2017

ThemeProvider extends React.Component... otherwise React is only needed for the tests. So... I think it's needed as a dependency.

No, tests need devDependency, code needs peerDependency, they both can co-exist.

@hburrows
Copy link
Collaborator

hburrows commented Oct 6, 2017

That's correct. I was thinking peerDependency but wrote dependency. I need another cup of coffee!!

@hburrows
Copy link
Collaborator

@kof What needs to be done to get someone to pay attention to this? I'm happy to help out but can only do so if the maintainers are receptive. I see all the great work you're trying to do with react-jss but not keeping up with the latest version of React sends a bad signal and having a critical dependency on a project that appears abandoned doubles the concern. You can always fork this project if the maintainers have no interest in keeping it current.

@hburrows
Copy link
Collaborator

@iamstarkov @oliviertassinari I'm happy to help make this repo compatible with React 16. Please let me know how I can contribute. @oliviertassinari don't you care about this for material-ui v1 ?

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Oct 10, 2017

@hburrows Material-UI doesn't rely on the package.

@hburrows
Copy link
Collaborator

hburrows commented Oct 10, 2017

@oliviertassinari material-ui v1-beta depends on react-jss@7.2.0 and that package dependents on theming@1.1.0. Doesn't that mean material-ui indirectly depends on theming? Maybe I'm misunderstanding something.... but when I build my project using react-jss with react 16 I end up with 2 version of react - 16 and 15.6.2.

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Oct 10, 2017

@hburrows Right, Material-UI indirectly dependent on this package at the installation time, nothing at the runtime.

@kof I think that the best path going forward is to remove this dependency from react-jss. Allowing more flexibility and ownership on this critical part of the styling solution.

@hburrows
Copy link
Collaborator

@oliviertassinari Thanks for the quick response. I get what you're saying. From react-jss I use JssProvider, SheetsRegistry, and injectSheet. When I build using React 16 I get 2 versions of React - maybe JssProvider or SheetsRegistry is the culprit (I'll investigate their dependencies). I'm using JssProvider and SheetsRegistry to create printable versions of pages -- something I would continue to do with material-ui v1 unless you're providing an alternative way of getting styles after calling ReactDOMServer.renderToStaticMarkup.

@kof I'm 👍 to removing theming as a react-jss dependency and taking ownership of it directly. That would at least allow me to fork theming without also having to fork react-jss to work around this until an alternative is found.

@kof
Copy link
Member

kof commented Oct 10, 2017

Theming is a separate package for a reason. It is a common effort from multiple cssinjs projects in order to unify theming. It is used by emotion and the plan was we all agreed upon to use it in styled-components and glamorous and hopefully more.

I guess the problem here is that nobody did the work of migrating as of right now.

I have publishing rights in this package, so I will fix this at some point if nobody else does. In the meantime everyone is free to fork both projects and use them temporarily.

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Oct 10, 2017

@kof I can't see the win for react-jss. I'm very grateful to @iamstarkov for sharing his implementation. This saved me a lot of time with implementing a custom version for Material-UI. I have soon reached the limitation of the abstraction. By the way, I have opened three issues to provide feedback on those limitations. This plan doesn't seem to move forward (while the importance of any problem tends to increase over time):

I think that it's time to move on, it's just my humble opinion.

@kof
Copy link
Member

kof commented Oct 10, 2017

I will try to get the admin rights for the repo and add more people so we can all move it forward. Otherwise we will just fork and move forward.

@iamstarkov
Copy link
Member

i'll take care of everything

@iamstarkov
Copy link
Member

i moved repo to cssinjs org

@iamstarkov
Copy link
Member

so @kof is the admin now

@iamstarkov
Copy link
Member

and ask him to add active contributors

@iamstarkov
Copy link
Member

i think i cant handle long term oss, and im deeply sorry for you. i wish i would delegate the project earlier

@kof
Copy link
Member

kof commented Oct 14, 2017

Don't worry @iamstarkov, get better and come back any time.

@hburrows
Copy link
Collaborator

@kof Submitted a PR (#45) to upgrade dependencies and make compatible with React 16.

@kof kof closed this as completed in #45 Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants