[core] Continue eslint sync with main repo #2004

oliviertassinari · 2020-07-18T13:01:30Z

Follow-up on #1930 and mui/material-ui#21758.

I turned a couple of rules as "warn" as I didn't want to handle them now. We can take care of them progressively, one that is concerning and important to fix is the import/no-cycle one.

Same as mui/mui-x#72. Once merged, we will need to work on solving all the warnings, this is a pre-requisite to be able to move the component in the main repository.

vercel · 2020-07-18T13:01:35Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/3czavooqp
✅ Preview: https://material-ui-pickers-git-fork-oliviertassinari-eslint-core.mui-org.vercel.app

oliviertassinari · 2020-07-18T13:02:10Z

package.json

+    "/docs/",
+    "/e2e/",


Legacy, no need to handle them

cypress · 2020-07-18T13:06:48Z

Test summary

78 • 0 • 3 • 0

Run details


Project	material-ui-pickers
Status	Passed
Commit	`5acd758`
Started	Jul 27, 2020 11:19 PM
Ended	Jul 27, 2020 11:20 PM
Duration	01:23 💡
OS	Linux Debian - 10.0
Browser	Chrome 80

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

dmtrKovalenko

There is one rule that I am strongly want to discard and never use nor in the picker nor in the core code – default exports. We should not use them when we exactly know what we are importing – in all cases except examples.

In short: when we exporting CalendarView we do want everybody to use them like CalendarView and not like Calendar or something else. This potential rename through the default exports is always implicit – why not avoid it? Moreover code with only named exports looks much more consistent because all the import lines have the same structure (starting with import {)

https://medium.com/@rajeshnaroth/avoid-es6-default-exports-a24142978a7a
https://humanwhocodes.com/blog/2019/01/stop-using-default-exports-javascript-module/

dmtrKovalenko · 2020-07-18T18:03:51Z

lib/src/DateRangePicker/DateRangePickerDay.tsx

+  const {
+    className,
+    day,
+    inCurrentMonth,
+    isEndOfHighlighting,
+    isEndOfPreviewing,
+    isHighlighting,
+    isPreviewing,
+    isStartOfHighlighting,
+    isStartOfPreviewing,
+    selected,
+    ...other
+  } = props;


What the original motiviation for this kind of change? Is this doing automatically?

The first-order motivation is to match the convention of the main repository. The second-order motivation was to allow quick scanning of the signature of the function. If you see "props" => it means it's a React component.

dmtrKovalenko · 2020-07-18T18:04:38Z

lib/src/LocalizationProvider.tsx

+  children: PropTypes.node,
 } as any;

-export default withDefaultProps({ name: 'MuiPickersLocalizationProvider' }, LocalizationProvider);
+export default LocalizationProvider;


Accidental changes?

What problem does withDefaultProps solve? I have removed it as LocalizationProvider is already a global provider, it duplicates.

lib/src/index.ts

dmtrKovalenko · 2020-07-18T18:13:39Z

lib/src/CalendarSkeleton.tsx

    const classes = useStyles();
    const calendarClasses = useCalendarStyles();

    return (
      <div className={clsx(classes.root, className)} {...other}>
        {monthMap.map((week, index) => (
          <div key={index} className={calendarClasses.week}>
-            {week.map((day, index) => (
+            {week.map((day, index2) => (


Why index2?

https://eslint.org/docs/rules/no-shadow

lib/src/DateRangePicker/DateRangePickerViewDesktop.tsx

oliviertassinari

Interesting topic on the named vs default imports, thanks for bringing it up. A couple of thoughts:

I don't understand the reasoning of your argumentation. Would you like to make the case for it with an RFC on the main repository? Detailing the current problems and how this potential new standard could fix them?
Both articles start by mentioning that both are valid, that it's a coding convention before anything else

This is opinionated nit picky territory.

Personal preferences

Looking at the thread on Twitter: https://twitter.com/slicknet/status/1084101377297506304. It seems that default vs named export isn't the core of the solution of the problems, but the solution is much more about having a convention and following it systematically.
Looking at the problems raised by the articles, it seems it can be solved with the convention we applying in the main repository for the React components: one component = one file, name of the component = name of the file. As for non-React modules, that's a different story.

oliviertassinari · 2020-07-18T18:28:16Z

lib/src/CalendarSkeleton.tsx

    const classes = useStyles();
    const calendarClasses = useCalendarStyles();

    return (
      <div className={clsx(classes.root, className)} {...other}>
        {monthMap.map((week, index) => (
          <div key={index} className={calendarClasses.week}>
-            {week.map((day, index) => (
+            {week.map((day, index2) => (


https://eslint.org/docs/rules/no-shadow

oliviertassinari · 2020-07-18T18:29:45Z

lib/src/DateRangePicker/DateRangePickerDay.tsx

+  const {
+    className,
+    day,
+    inCurrentMonth,
+    isEndOfHighlighting,
+    isEndOfPreviewing,
+    isHighlighting,
+    isPreviewing,
+    isStartOfHighlighting,
+    isStartOfPreviewing,
+    selected,
+    ...other
+  } = props;


The first-order motivation is to match the convention of the main repository. The second-order motivation was to allow quick scanning of the signature of the function. If you see "props" => it means it's a React component.

oliviertassinari · 2020-07-18T18:30:57Z

lib/src/LocalizationProvider.tsx

+  children: PropTypes.node,
 } as any;

-export default withDefaultProps({ name: 'MuiPickersLocalizationProvider' }, LocalizationProvider);
+export default LocalizationProvider;


What problem does withDefaultProps solve? I have removed it as LocalizationProvider is already a global provider, it duplicates.

lib/src/index.ts

dmtrKovalenko · 2020-07-18T19:13:21Z

I don't understand the reasoning of your argumentation

I have provided 3 arguments:

Implicit renames that could be buggy and provide unobvious
The clearness in the module dependencies
Easier refactoring
More solid imports sections
Ability to avoid unnecessarily "verbal" convention

Ok, I`ll open RFC for the core repository. On Monday 😎

oliviertassinari · 2020-07-18T19:27:48Z

Ok, I`ll open RFC for the core repository. On Monday 😎

Perfect

oliviertassinari · 2020-07-19T09:47:41Z

On a side note, there is an important eslint rule that I couldn't fix and I had to put as a warning that will require to rethink the file structure: import/no-cycle. @dtassone Thanks for insisting on this rule. It turns out +23s on the main repository build was worth it for here :).

I think that it will be a perfect opportunity to adopt the main repository file structure (folders, index.js, default export, etc). It might also something we want to batch with the tree-shaking reports: #1670.

I have reverted the changes related to the import, waiting for the outcome of the RFC. It should be ready now

oliviertassinari · 2020-07-28T11:47:28Z

@dmtrKovalenko Can I merge before we get a conflict?

dmtrKovalenko · 2020-07-28T11:48:21Z

Yes you can :)

oliviertassinari added the core Infrastructure work going behind the scene label Jul 18, 2020

vercel bot deployed to Preview July 18, 2020 13:01 View deployment

oliviertassinari commented Jul 18, 2020

View reviewed changes

package.json Outdated

Comment on lines 65 to 71

"/docs/",

"/e2e/",

Copy link

Member Author

oliviertassinari Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legacy, no need to handle them

oliviertassinari force-pushed the eslint-core branch from 612f50f to 761cab7 Compare July 18, 2020 13:22

vercel bot temporarily deployed to Preview July 18, 2020 13:22 Inactive

oliviertassinari force-pushed the eslint-core branch from 761cab7 to 6ffcced Compare July 18, 2020 13:25

vercel bot deployed to Preview July 18, 2020 13:26 View deployment

oliviertassinari force-pushed the eslint-core branch from 6ffcced to 9b3f7b9 Compare July 18, 2020 13:29

vercel bot deployed to Preview July 18, 2020 13:29 View deployment

oliviertassinari requested a review from dmtrKovalenko July 18, 2020 13:34

oliviertassinari mentioned this pull request Jul 18, 2020

[core] Continue eslint sync with main repo mui/mui-x#72

Merged

dmtrKovalenko previously requested changes Jul 18, 2020

View reviewed changes

dmtrKovalenko reviewed Jul 18, 2020

View reviewed changes

lib/src/index.ts Outdated Show resolved Hide resolved

dmtrKovalenko reviewed Jul 18, 2020

View reviewed changes

oliviertassinari commented Jul 18, 2020

View reviewed changes

oliviertassinari mentioned this pull request Jul 19, 2020

Bump @typescript-eslint/eslint-plugin from 1.13.0 to 3.6.1 #2013

Closed

oliviertassinari force-pushed the eslint-core branch from 9b3f7b9 to 68cd77b Compare July 19, 2020 09:16

vercel bot deployed to Preview July 19, 2020 09:16 View deployment

dmtrKovalenko mentioned this pull request Jul 20, 2020

[RFC] Avoid or "prefer not to use" default exports mui/material-ui#21862

Open

oliviertassinari force-pushed the eslint-core branch from 68cd77b to c55d4c6 Compare July 25, 2020 22:22

vercel bot deployed to Preview July 25, 2020 22:22 View deployment

oliviertassinari force-pushed the eslint-core branch from c55d4c6 to cf7f555 Compare July 25, 2020 23:15

vercel bot deployed to Preview July 25, 2020 23:15 View deployment

oliviertassinari force-pushed the eslint-core branch from cf7f555 to b07e758 Compare July 25, 2020 23:44

vercel bot deployed to Preview July 25, 2020 23:44 View deployment

oliviertassinari force-pushed the eslint-core branch from b07e758 to 2906640 Compare July 25, 2020 23:58

vercel bot temporarily deployed to Preview July 25, 2020 23:58 Inactive

oliviertassinari force-pushed the eslint-core branch from 2906640 to b81f88f Compare July 25, 2020 23:59

vercel bot temporarily deployed to Preview July 25, 2020 23:59 Inactive

oliviertassinari force-pushed the eslint-core branch from b81f88f to 79f0eb1 Compare July 26, 2020 00:05

vercel bot temporarily deployed to Preview July 26, 2020 00:05 Inactive

oliviertassinari force-pushed the eslint-core branch from 79f0eb1 to a03468b Compare July 26, 2020 00:12

vercel bot deployed to Preview July 26, 2020 00:13 View deployment

oliviertassinari requested a review from dmtrKovalenko July 26, 2020 00:13

oliviertassinari force-pushed the eslint-core branch from a03468b to 712de04 Compare July 27, 2020 14:10

vercel bot deployed to Preview July 27, 2020 14:10 View deployment

oliviertassinari force-pushed the eslint-core branch from 712de04 to 68f2e2e Compare July 27, 2020 22:42

vercel bot temporarily deployed to Preview July 27, 2020 22:42 Inactive

oliviertassinari added 2 commits July 28, 2020 00:43

[core] Update eslint to match main repository

a5dc8aa

second iteration

ce97a21

oliviertassinari force-pushed the eslint-core branch from 68f2e2e to ce97a21 Compare July 27, 2020 22:43

vercel bot requested a deployment to Preview July 27, 2020 22:44 Pending

vercel bot deployed to Preview July 27, 2020 22:54 View deployment

rebase

5acd758

vercel bot deployed to Preview July 27, 2020 23:13 View deployment

oliviertassinari merged commit 537dc6e into mui:next Jul 28, 2020

oliviertassinari deleted the eslint-core branch July 28, 2020 11:51

mui deleted a comment from aamirafridi Jul 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Continue eslint sync with main repo #2004

[core] Continue eslint sync with main repo #2004

oliviertassinari commented Jul 18, 2020 •

edited

Loading

vercel bot commented Jul 18, 2020 •

edited

Loading

oliviertassinari Jul 18, 2020

cypress bot commented Jul 18, 2020 •

edited

Loading

dmtrKovalenko left a comment •

edited

Loading

dmtrKovalenko Jul 18, 2020

oliviertassinari Jul 18, 2020

dmtrKovalenko Jul 18, 2020

oliviertassinari Jul 18, 2020

dmtrKovalenko Jul 18, 2020

oliviertassinari Jul 18, 2020

oliviertassinari left a comment •

edited

Loading

oliviertassinari Jul 18, 2020

oliviertassinari Jul 18, 2020

oliviertassinari Jul 18, 2020

dmtrKovalenko commented Jul 18, 2020

oliviertassinari commented Jul 18, 2020

oliviertassinari commented Jul 19, 2020 •

edited

Loading

oliviertassinari commented Jul 28, 2020

dmtrKovalenko commented Jul 28, 2020

[core] Continue eslint sync with main repo #2004

[core] Continue eslint sync with main repo #2004

Conversation

oliviertassinari commented Jul 18, 2020 • edited Loading

vercel bot commented Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

cypress bot commented Jul 18, 2020 • edited Loading

Test summary

Run details

dmtrKovalenko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko commented Jul 18, 2020

oliviertassinari commented Jul 18, 2020

oliviertassinari commented Jul 19, 2020 • edited Loading

oliviertassinari commented Jul 28, 2020

dmtrKovalenko commented Jul 28, 2020

oliviertassinari commented Jul 18, 2020 •

edited

Loading

vercel bot commented Jul 18, 2020 •

edited

Loading

cypress bot commented Jul 18, 2020 •

edited

Loading

dmtrKovalenko left a comment •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Jul 19, 2020 •

edited

Loading