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

Upcoming breaking changes (v14) #748

Closed
18 of 20 tasks
emmenko opened this issue Jun 14, 2019 · 17 comments · Fixed by #874
Closed
18 of 20 tasks

Upcoming breaking changes (v14) #748

emmenko opened this issue Jun 14, 2019 · 17 comments · Fixed by #874

Comments

@emmenko
Copy link
Member

emmenko commented Jun 14, 2019

In the next major release I would like to go with first support for hooks, which will help us to ship less and more performant code.
Therefore, we should:

  • support only react >16.8.0
  • use hooks in the appkit packages implementations, easier to type as well
  • I would also like to drop the HOC functions

We can maybe do the same in the upcoming uikit v10 and release both versions together.

Also maybe upgrade RTL.

Thoughts?


@tdeekens
Copy link
Contributor

I like it. Hope that people will update. However, I feel there are other reasons than bumping react why some projects do not get updated. To be fair, I don‘t think hooks for hocs will show meaningful performance improvements. It will make code easier to type and read for sure.

@emmenko
Copy link
Member Author

emmenko commented Jun 15, 2019

Yeah, for the performance I mean more from the React side of things. For example less wrapping components, probably react can even further optimize things internally (just a hunch from my side).
Also Babel will generate less code 🙌

@montezume
Copy link
Contributor

I would like to remove all of the usages HOCs from UI-Kit. withMouseOver etc can be done with pure css instead of using JavaScript.

@emmenko emmenko added this to the v14 milestone Jun 15, 2019
@montezume
Copy link
Contributor

Can we also make sure we include the assets renaming?

#546

🙏

@emmenko
Copy link
Member Author

emmenko commented Jun 24, 2019

Short update: I've been trying to migrate the notifications package to TS and typing redux middlewares is kind of a nightmare.
That led me to consider refactoring the package to use hooks (including redux hooks), which makes it also easier to type.
Therefore, I would like to include those changes in this new release, or at least from this release onwards.

@emmenko
Copy link
Member Author

emmenko commented Jul 1, 2019

Short update: I tried to use react apollo hooks (#801), which are supported by the upcoming react-apollo v3 (currently in beta). This lead to having to update react to 16.9 (currently in alpha) and RTL to latest (@testing-library/react).
Upgrading those dependencies require some extra work. We also need to coordinate how we should migrate the mc-fe apps to those versions.

We might decide to skip those updates in the v14 release but we will eventually need to do that in a next major (for instance, uikit should do the same).

@montezume
Copy link
Contributor

Hm, UI-Kit already uses the latest @testing-library/react, and it doesn't use apollo. 😕

@emmenko
Copy link
Member Author

emmenko commented Jul 1, 2019

Ah ok...it's also react to be updated...but anyway, as long as we keep in sync it should not matter

@tdeekens
Copy link
Contributor

tdeekens commented Jul 1, 2019

I am being a bit pungent again. Should we finish the static typing story and then move on to this [working on releasing v14]. Sometimes I feel we just jump onto whatever we feel would be most fun or interesting but also beneficial. However, we should put ourselves to the same standards and ideals we communicate towards others: finish before starting.

@montezume
Copy link
Contributor

@tdeekens by this are you referring to updating mc-fe to latest RTL, or are you referring to releasing a major app kit release?

@tdeekens
Copy link
Contributor

tdeekens commented Jul 1, 2019

Sorry, for releasing a major version of app-kit. I am all for planning it and thinking about it. Still, I wonder why we start it before we finish the static typing story. In all splitting and jumping between those "projects" just diffuses energy and effort and increases the likihood of leaving either or in a have finished state.

@montezume
Copy link
Contributor

Well, we need a major version of app kit to update UI-Kit though, and we have spent a lot of energy preparing for the next major UI-Kit...

@montezume
Copy link
Contributor

I understand your concerns about starting new things and not finishing, but we can't suffer from release paralysis. Sometimes we need to release breaking changes, and some of the time it's not even coming from the developers side, but from the designers, etc.

@tdeekens
Copy link
Contributor

tdeekens commented Jul 1, 2019

Sometimes we need to release breaking changes, and some of the time it's not even coming from the developers side, but from the designers, etc.

It's not about breaking changes. It's about focusing in on something else started and finishing it. If this now became more important as UIKit depends on it, then that's a valid reason to not work on typing more right now.

@emmenko
Copy link
Member Author

emmenko commented Jul 1, 2019

The static typings was something we agreed on doing "on the side", as it's not a high priority task. The v14 release is more important to finish first, as it brings more direct benefits.

With that in mind, in general start finishing before starting still stands of course!

@montezume
Copy link
Contributor

We can also progressively (on the side if you will) migrate App Kit to hooks once the breaking change is released requiring React 16.8. 😄

@emmenko
Copy link
Member Author

emmenko commented Jul 1, 2019

Should we include latest RTL in this release? I don't think we can really upgrade the mc-fe separately, as the app-shell test-utils rely on a specific rtl version.

@emmenko emmenko pinned this issue Jul 15, 2019
@emmenko emmenko mentioned this issue Jul 22, 2019
@emmenko emmenko unpinned this issue Jul 23, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants