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

[test] Add yarn deduplicate step #107

Merged
merged 1 commit into from
Jul 27, 2020
Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jul 25, 2020

One chunk of #37, copy & paste from the main repository.

@oliviertassinari oliviertassinari mentioned this pull request Jul 25, 2020
13 tasks
@oliviertassinari oliviertassinari marked this pull request as ready for review July 25, 2020 17:32
@dtassone
Copy link
Member

Do we really need that? Yarn already dedup...
Also by introducing this package, we would have a different setup from our users, hence they might see issues that we won't see due to this extra step in the package management 🤔

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 27, 2020

Yarn already dedup...

Does he? https://github.com/atlassian/yarn-deduplicate#why-is-this-necessary. Proof: mui/material-ui-pickers#2036.

Do we really need that?

It's important for: avoiding types duplication and wrong version resolutions, bundle bloat by duplicated version. @eps1lon anything else?

we would have a different setup from our users

What issues do you have in mind? We should already design knowing Material-UI can be included twice in a bundle. It would also be better to have a dedicated test case for it if we want to give it a lot of weight.

@eps1lon
Copy link
Member

eps1lon commented Jul 27, 2020

@eps1lon anything else?

Keeping install size/time low. Every package adds a bit of size(=time) and in the npm ecosystem this actually results in a death by thousand cuts.

Ideally we'd autofix it but for human PRs I'd rather not let bots commit branches. For dependabot PRs we should. I do this on some of my repos which worked quite nice. Now that GH actions are public I should revisit that since human time > machine time.

Do we really need that? Yarn already dedup...

It does when installing but if you have a lockfile then every package manager makes sure that unrelated dependencies are not changed (i.e. deduped). So if you have A -> B -> Dependency, A -> Dependency and bump A/Dependency you don't want to bump A/B/Dependency since you didn't declare that you also want to change B. This is because dependencies are not part of you public API. It doesn't matter that B depends on Dependency. That is the implementation not the interface.

@dtassone
Copy link
Member

avoiding types duplication, and wrong version resolutions

Yes and that can happen if we mismanage our versions, which can create issues on our local if we have duplicated types.

If one of the x-grid-modules installed package is duplicated for any reason, this extra step will delete the extra package.
But yarn or npm won't do it natively for our users. So it will leave us not understanding the bug our users are facing as we won't have the same setup.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 27, 2020

@dtassone I personally don't recall seeing a GitHub issue report coming from such a problem. We have one similar case not directly related, but coming from one of our singletons with @material-ui/styles (we solved it by encouraging peer dependencies on core). I don't think that there is any downside to the change. I have a high confidence rate that this potential issue won't cause us any problem, that: developers are more likely to report the issue than we are to catch it with our repository. If it doesn't, we can point back to my comment at how I was wrong and you were right :). It's not like we are going to have a lot of dependencies anyway, considering you don't want to depend on clsx but bundle it :p.

@oliviertassinari oliviertassinari merged commit 1865ee2 into master Jul 27, 2020
@oliviertassinari oliviertassinari deleted the add-yarn-deduplicate branch July 27, 2020 20:04
dtassone added a commit that referenced this pull request Jul 28, 2020
* Release v0.1.62 (#95)

* v0.1.51

* fix demo app build

* v0.1.52

* v0.1.53

* v0.1.54

* v0.1.55

* v0.1.56

* v0.1.57

* v0.1.58

* v0.1.59

* add missing export

* v0.1.60

* v0.1.61

* hot fix for tsconfig conf

* fix mistypes on the package.json

* fix empty space with prettier

* v0.1.62

* fix lerna.json whitespace

* fix resize issue (#96)

* Bump eslint-config-airbnb-typescript from 8.0.2 to 9.0.0 (#112)

* Pagination refactoring (#100)

* implemented server pagination

* rename pagination options, doc, prettier

* fix lint

* Apply suggestions from code review

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* change casing value in featureMode and remove apiref check

* added missing dep in useEffect

* fix broken suggestion code

* fix doc comments

* Apply suggestions from code review

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* Fix FeatureMode enum and remove useCallback in demo

* fix import order

* added namespace to function call to remove import

* remove useCallback and add react. to mdx code

* add concurrency in server pagination demo

* fix feature mode export

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* [core] Use the classnames helper (#105)

* [core] Batch small changes (#104)

* [docs] Improve the JSDoc (#106)

* rename events and add onHover event (#103)

* rename events and added onHover event

* rename events and added onHover event

* fix onPageChange rename and merge

* Js doc improvements

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* added hover stories

* small fix and cleanup

* revert rename, back to onRowSelected

* refactor hover and click handlers

* refactor cellParams

* prettier

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* [test] Add yarn deduplicate step (#107)

* [DataGrid] Set default logLevel to warn (#117)

* override default logLevel to warn

* rerun CI

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* [test] Lint JSON (#108)

* Merging release back to master (#120)

* v0.1.51

* fix demo app build

* v0.1.52

* v0.1.53

* v0.1.54

* v0.1.55

* v0.1.56

* v0.1.57

* v0.1.58

* v0.1.59

* add missing export

* v0.1.60

* v0.1.61

* hot fix for tsconfig conf

* fix mistypes on the package.json

* fix empty space with prettier

* v0.1.62

* fix lerna.json whitespace

* v0.1.63

* v0.1.64

* prettier lerna.json due to lerna

* fix cellClassRule type, fix keyboard timeout, small refactoring (#123)

* fix cellClassRule type, fix keyboard timeout, small refactoring

* prettier fix

* add cancel RAF for focus if new event coming

* added keyboard doc (#125)

* added keyboard doc

* Apply suggestions from code review

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* [DataGrid] Fix selection issue (#126)

* fix selection issue

* fix selection issue test

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
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.

None yet

3 participants