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

[RFC] Integrate the pickers repository back into the main mono-repository #19706

Closed
oliviertassinari opened this issue Feb 14, 2020 · 9 comments · Fixed by #22692
Closed

[RFC] Integrate the pickers repository back into the main mono-repository #19706

oliviertassinari opened this issue Feb 14, 2020 · 9 comments · Fixed by #22692
Labels
component: date picker This is the name of the generic UI component, not the React module! priority: important This change can make a difference
Milestone

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 14, 2020

Context

A bit of history, Material-UI has had an agitated history with the date picker. It's a component we used to support very early in the core, at least, 4 years ago: https://v0.material-ui.com/#/components/date-picker.
However, during the v1 rewrite, it was, at some point, decided that the core team didn't have the bandwidth for it, that it would be better to distribute the ownership. So we decided to leave to the component to the community. It's where @dmtrKovalenko came in as a savior and filled the gap ✨ with material-ui-pickers. Over the following months, the core team kept strengthening its link with this project, up to a point where Material-UI now financially supports Dmitriy to work on the project's mission, as of now with a major upgrade of the Date Picker (to v4).

Looking back, we have a list of signals that suggest the date picker is a key component in any UI library:

  • The date picker alone has as many downloads on npm as all the lab components combined.
  • The 8th most upvoted issue on our repository.
  • The date range feature is the 2nd most upvotes issue we have seen (behind styled-components).
  • Given the complexity of the component, it's rearely something any product team build. When they do, it's rarely great. It can be a "trojan horse" for Material-UI to gain adoption. I have seen it unfold for antd in Closes #2396: Add finer-grained scheduling. getredash/redash#2426 (comment).

Proposal

In this context, I propose that we merge https://github.com/mui-org/material-ui-pickers into https://github.com/mui-org/material-ui, under the following location: /packages/material-ui-pickers.

Why?

The potential we are missing on: a better integration between the pickers and the core components. https://en.wikipedia.org/wiki/Monorepo provides a good chunk of information on the tradeoff.

From my perspective, it unfolds into:

  1. Easier code sharing. Catch bugs between core and pickers on the earliest stage. Empower large scale code refactorting.
  2. Easier knowledge sharing. Silos less likely. Consistant API.
  3. Consistent documentation. Same UX for the users (demos, API, Algolia etc). If you run the query "react datepicker" on Google (in FR), you will find vuetify date picker (page 7) before you find the documentation (material-ui-pickers.dev).
  4. Easier tools sharing. We setup all the dev dependencies and CI/CD tools once.
  5. An opportunity to experience new approaches. For instance, from the main repo perspective: components written in TypeScript, Cypress, Percy, Now, etc.

Potential downsides

  1. Releases. We will need to sync releases of pickers and core, sync versions which can be a problem on the earliest stages of v4, when possibly we will need to make some patches. However, using a different package @material-ui/pickers solve most of the problem. @eps1lon has proposed to move away from a single release (in terms of GitHub) to multiple ones. So I think that it’s up to us to figure the strategy that work well.
  2. Longer CI. Will we need to run all tests for the pickers? We already faced this problem in the past. For instance, we only run the tests of the icons package if the source change. Maybe we could use something similar?
  3. Mixed issues and PRs. This is my main point of interogation. I think it would have the biggest impact on @dmtrKovalenko. He will, all a sudden, be "flooded" with the main repository issues flow, it will be overwhelming, at the beginning. On the core side, it wouldn’t made a significant difference.
    If only GitHub had this feature https://github.community/t5/How-to-use-Git-and-GitHub/Feature-Request-Subscribe-to-notifications-for-specific-Labels/td-p/16583!
    Now, I suspect that if Tensorflow can handle it with x6 more weekly contributors than us and VScode x6 more weekly issues than us, we can probably too with a mono-repository.
    If it gets out of hand. We can break down the mono-repository later, once our scale forces it? But I don’t know if a single component will be responsible for a large flow issues. I suspect is will be a diffuse, in which case, how do we break it down? I wish this is a problem we will have to solve in the future, it would imply so much about our impact 😍.
  4. Large effort potentially required to merge the sources. No pain no gain.
@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2020

Easier code sharing. Catch bugs between core and pickers on the earliest stage.

has this become a problem? I haven't noticed them apart from some minor TS issues that are not tied to /pickers anyway.

I'm missing a tolling comparison. Someone should compare CI and see what overlaps and what takes additional time.

Then we need a concrete plan of action. This is still a very shallow explainer. What actual steps would we take to merge the repos? docs first? source first? What happens with the history? Transfering issues/PRs? When will the old repo be archived?

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Feb 16, 2020

Basically we need to have different CI. Different tests, build and everything.
My thought on that — we can make 2 branches of CircleCI workspaces for pickers and core and basically early from core branch if only pickers was changed.

Pickers are quite complex control because we are testing everything for each of date-adapters. AFAR Now we have something like 1200 integration (enzyme) tests and 60 cypress tests per run.

@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2020

Basically we need to have different CI. Different tests, build and everything.

This is not describing a monorepo. If we have to have different "everything" then separate repositories is the way to go.

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Feb 16, 2020

Totally, but the main idea is to merge documentation for better SEO and potentially easier for users to find information and examples (1 site instead of 2)
It is impossible to merge documentations without merging sources

@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2020

So it's just point 3? Point 1, 2, 4 and 5 made by Olivier don't apply? Has somebody looked at the viability of pulling in all the other tools? How do we merge it? What happens to history etc.

This seems all very shallow. "just merge it into packages/material-ui-pickers" is not enough. CI would probably slow down by 50%. Not to mention how this would even integrate regarding git history.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 17, 2020

Here is an extended version of the previous advantages:

  1. code sharing. This is probably the single most important argument I have, the following ones derive from it. It's a profound belief that the fewer barriers we have between the different modules of our software, the more consistent user experience and contribution experience we will provide, the more efficient and effective we will be, the better our software will be. Can I prove it? No, but this strategy been working great for us so far, I suspect we can apply it to the date picker too.
  2. knowledge sharing:
  • They are API inconsistencies between the core and the date picker in v3, for instance, allowKeyboardControl's default value that goes against https://material-ui.com/guides/api/#property-naming.
  • They are knowledge sharing silo issues, for instance, the missing usage of forwardRef, of withStyles, of describeConformance, etc. I believe we can reduce the scope of this issue by having contributors that work on the date picker as well as the other components (and maybe the only solution). Merging the repositories makes this double contribution scope more likely.
  1. docs. Great, we agree. One good reason is often enough :).
  2. tools. They are tool sharing issues. For instance, We don't use the same eslint or prettier configuration. We don't use the same GitHub bots, labels. We don't use the same bundle size reporter. Any small improvement we make to any of these tools doesn't compound with all our components. We miss potential.
  3. experiment. I think that experimenting with new tools is important, it gives us an opportunity to reconsider the ones we have, to try them in practice.
    I'm very eager to try a TypeScript based component. I'm eager to try how we could write better tests with Cypress, for cases where karma isn't enough. For instance, to test the keyboard accessibility of the rating component.
  4. users's perception. From the Survey we ran with Dmitriy late last year.
    Capture d’écran 2020-02-17 à 19 59 54
    What do you think about merging pickers package into core? 100 answers

I'm missing a tolling comparison. Someone should compare CI and see what overlaps and what takes additional time.

Regarding the tooling comparison, I would propose that we keep the following:

  • TypeScript
  • Cypress

Regarding the other tools that we don't already have in core and that I would propose to drop:

  • Now. As far as I understand it, it was introduced to support dynamic server-side rendering. Great to avoid the light/dark theme flash. However, it has two implications I'm worried about 1. performance, can we cache the result? (I assume, we can but it's critical for SEO, to double-check) 2. what about the cost, if I can trust SimilarWeb data, we have ~x50 more sessions on material-ui.com. It's not the same scale and will likely continue to grow. A side note, Gatsby also has this flash issue, I believe they went for generating the light and dark CSS at the same time. I also have a concern with the nosiness of the comments on GitHub.
  • Percy. The free plan won't get us far (5k screenshots). On Argos, we push ~680 builds a month and compare ~200k screenshots a month. Argos has been unreliable lately, I suspect because the database is almost full and shared. We could host it under our own servers for about $80 / month, about the same amount we pay Netlify.
  • Bundlesize. Sebastian made a significant upgrade to the tool.

Tools I'm unsure about:

  • Jest. I have used it a couple time in the past, great experience, but how does it fit with mocha? Do we need it? I would have a concern with using different expect API, it could be confusing.

Did I miss any other tool?


Then we need a concrete plan of action. This is still a very shallow explainer. What actual steps would we take to merge the repos? docs first? source first? What happens with the history? Transfering issues/PRs? When will the old repo be archived?

Agree. The objective was to make abstraction of the current setup and to emphasize where we went to go, long term. Both projects are built around authoring components, so I have assumed we could migrate the code in any direction.

In practice, they are the following to consider:

Capture d’écran 2020-02-17 à 21 00 04

  • CI/CD. This would be the biggest unknown. We would need to run prettier, make the eslint changes required, make the TypeScript changes required, update the tests to run, update the API generation script to run.

Basically we need to have different CI. Different tests, build and everything.
My thought on that — we can make 2 branches of CircleCI workspaces for pickers and core and basically early from core branch if only pickers was changed.
Pickers are quite complex control because we are testing everything for each of date-adapters. AFAR Now we have something like 1200 integration (enzyme) tests and 60 cypress tests per run.

@dmtrKovalenko No objection with that, I believe we already have different runs for the icons. Regarding the CI duration. The pickers seem to add a ~6 minutes serial overhead. We have a couple of leverages.

  1. Test the components with a single date engine, the most used one, date-fns. Test each date engine API independently. => save 2 minutes
  2. Only run cypress if the source of material-ui-pickers changes => save 3 minutes
  3. Pay CircleCI to have one more concurrency, it can absorb these 6 minutes. However, I suspect it's not even needed. The pricing modal has changed, its credit-based now. In the current configuration, we use 135k credits/month, our OSS plan has 400k credits/month, pickers included!
    Can somebody try our build with more concurrency and see if we have it? maybe @eps1lon?

But as we grow the number of components we support the slowness of the CI will be an issue anyway. I wonder, can Jest be smart about the dependencies of the tests and only run the changes that were impacted?

@eps1lon
Copy link
Member

eps1lon commented Feb 18, 2020

Jest. I have used it a couple time in the past, great experience, but how does it fit with mocha? Do we need it? I would have a concern with using different expect API, it could be confusing.

Can't use jest with karma. It's unfortunately not possible to use jest with our testing approach.

It seems like most of this effort has to happen in pickers. Their repository needs to integrate our tooling and folder structure until it looks similar to ours. It basically needs to mimic our monorepo but with a single package in packages/. And the package.jsoin in the manifest need to have the same dependencies.

Then we can think about merging.

When evaluating CI it would help as well if pickers would slowly migrate to our approach (CircleCI jobs, next+netlify). My guess is that netlify isn't that big of an issue. It would only add a couple of demo and API pages. Nothing too big.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 19, 2020

Can't use jest with karma. It's unfortunately not possible to use jest with our testing approach.

@eps1lon Do you think that it's a tradeoff we should reconsider?

Their repository needs to integrate our tooling and folder structure until it looks similar to ours.

Well, I think that we are all in the same boat 🤗. "their"/"ours" is what I had in mind by "avoiding silos" :). However, I agree with the underlying logic, the mono-repository will need some changes to adapt to the pickers' packages, but I would expect more effort needed on the pickers' side.


@dmtrKovalenko I think that a good approach would be to try the brut force approach, try the merge in a single pass. I doubt it will make it but "hitting the wall" should surface the implications and blocking elements. Then, we can re-evaluate / work on these blockers independently.

@dmtrKovalenko
Copy link
Member

@eps1lon - I can do all the work of CI integration by myself.
We do not need to merge stacks right now. I am thinking about speeding up CI by skipping parts if something not changed.
If we will merge sources – our current documentation (material-ui-pickers.dev) can be deleted anyway, so we will not need to use Zeit now

@oliviertassinari oliviertassinari added the component: date picker This is the name of the generic UI component, not the React module! label May 9, 2020
@oliviertassinari oliviertassinari added this to the v5 milestone Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: date picker This is the name of the generic UI component, not the React module! priority: important This change can make a difference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants