[DataGrid] Fix typings and packages assets by dtassone · Pull Request #339 · mui/mui-x

dtassone · 2020-09-22T13:54:33Z

Closes #343
Closes #38

oliviertassinari · 2020-09-22T14:39:21Z

So the solution was to treat @material-ui/x-grid-module as a regular folder? Should we remove its package.json? Also, should we remove the build step from
https://github.com/mui-org/material-ui-x/blob/009577f1de980aeea897de34a42d721e6b668734/package.json#L31?

oliviertassinari · 2020-09-22T14:52:46Z

Does using https://pkg.csb.dev/mui-org/material-ui-x/commit/93ab1549/@material-ui/data-grid for @material-ui/data-grid in the reproduction solve the issue? I haven't tried.

The reproduction: typescript-issue.zip.

oliviertassinari

The reproduction still fails with the latest commit on my side (https://pkg.csb.dev/mui-org/material-ui-x/commit/b5d0f060/@material-ui/x-grid). If I run yarn lerna:build locally, I get the following:

$ cat packages/grid/x-grid/dist/x-grid.d.ts
export * from '@material-ui/x-license';
import { GridComponentProps } from '@material-ui/x-grid-modules';
export * from '@material-ui/x-grid-modules';
import { MemoExoticComponent, ForwardRefExoticComponent, RefAttributes } from 'react';
[...]

oliviertassinari · 2020-09-22T16:00:43Z

I have opened mui/material-ui#22697 to have a confirmation of #339 (review) and help iterate.

fix prettier

oliviertassinari

We are getting closer:

@material-ui/data-grid

x-grid-data-generator leaks with one of the two packages

$ tsc -p tsconfig.json
node_modules/@material-ui/x-grid-data-generator/dist/esm/x-grid-data-generator.d.ts:1:43 - error TS2307: Cannot find module '@material-ui/x-grid' or its corresponding type declarations.

1 import { CellParams, ColDef, RowId } from '@material-ui/x-grid';
                                            ~~~~~~~~~~~~~~~~~~~~~

@material-ui/x-grid

x-license needs to be unbundled and published on npm

$ tsc -p tsconfig.json
node_modules/@material-ui/x-grid/dist/x-grid.d.ts:2:29 - error TS2307: Cannot find module '@material-ui/x-license' or its corresponding type declarations.

2 export { LicenseInfo } from '@material-ui/x-license';
                              ~~~~~~~~~~~~~~~~~~~~~~~~

oliviertassinari · 2020-09-23T15:49:45Z

+          targets: ['./data-grid/dist/'],
        }),
-      typescript({ tsconfig: 'tsconfig.build.json' }),
+      typescript({ tsconfig: 'tsconfig.json' }),


I think that we need two different tsconfig files:

Suggested change

typescript({ tsconfig: 'tsconfig.json' }),

typescript({ tsconfig: 'tsconfig.build.json' }),

One to lint the sources (a.) and a second one to build (b.). For instance, with (a.) we want to lint the end-to-end tests and the TypeScript tests, with (b.) we don't want to build the definitions for them.

I understand how the 2 files are used.
Not sure we need the one for linting.

Why wouldn't we need one for linting? The main reason I see is different include/excludes configuration.

because at the root you specify to lint every files with ts js tsx extensions, with no-emit, and it uses the eslint ignore
same inside packages
the only place it is used it's for the yarn typescript command which is just used to check the test as we exclude them in the build

Sorry, I wasn't clear. There are two linting stages: eslint and TypeScript. These are complementary. I was referring to the latter.

From what I know:

lint with eslint is configured at the root and ran with a single command (yarn eslint).

lint with TypeScript is run in parallel with Lerna and defined in each package (yarn typescript).

the only place it is used it's for the yarn typescript command which is just used to check the test as we exclude them in the build

yarn typescript tests all the demos of the documentation, all the end-to-end tests, all the type definition tests. Without this command, we would get silent fails (not reported in the CI).

dtassone · 2020-09-23T16:15:09Z

We are getting closer:

@material-ui/data-grid

x-grid-data-generator leaks with one of the two packages

$ tsc -p tsconfig.json
node_modules/@material-ui/x-grid-data-generator/dist/esm/x-grid-data-generator.d.ts:1:43 - error TS2307: Cannot find module '@material-ui/x-grid' or its corresponding type declarations.

1 import { CellParams, ColDef, RowId } from '@material-ui/x-grid';
                                            ~~~~~~~~~~~~~~~~~~~~~

@material-ui/x-grid

x-license needs to be unbundled and published on npm

$ tsc -p tsconfig.json
node_modules/@material-ui/x-grid/dist/x-grid.d.ts:2:29 - error TS2307: Cannot find module '@material-ui/x-license' or its corresponding type declarations.

2 export { LicenseInfo } from '@material-ui/x-license';
                              ~~~~~~~~~~~~~~~~~~~~~~~~

Where did you run this command from?
x-license is unbundled, but we still use it. It needs to be republished

oliviertassinari · 2020-09-23T16:17:48Z

Where did you run this command from?

Open codesandbox-ci, get the dependency URLs
Use the dependencies URLs in the issue reproduction (see the pull request's description)
yarn
yarn typescript

💥

oliviertassinari · 2020-09-24T10:37:09Z

I have tried the Lerna release workflow. It doesn't work: https://unpkg.com/browse/@material-next/[email protected]/. Tested on my fork.

oliviertassinari · 2020-09-24T11:01:41Z

#339 (comment) my bad, it works :).

On a different note I have noticed that TypeScript isn't linting /material-ui-x/packages/grid/x-grid/src/XGrid.test.tsx. It seems to be coming from before the changes in this pull request, so it's not a regression. Probably for another time.

oliviertassinari

The only issue I could spot is the missing x-license dependency, otherwise, all good.

oliviertassinari · 2020-09-24T11:46:44Z

🎉 the build is almost green using the latest commit here: mui/material-ui#22697. There is an issue with the import of the events Node.js library that make TypeScript assumes that setTimeout returns a Node.js reference. But I guess we can ignore it, for now. I'm patching the demos in the documentation.

dtassone requested a review from oliviertassinari September 22, 2020 13:54

dtassone changed the title ~~fix typings and packages assets~~ [DataGrid] fix typings and packages assets Sep 22, 2020

dtassone added the type: bug It doesn't behave as expected. label Sep 22, 2020

oliviertassinari added the typescript label Sep 22, 2020

oliviertassinari changed the title ~~[DataGrid] fix typings and packages assets~~ [DataGrid] Fix typings and packages assets Sep 22, 2020

oliviertassinari reviewed Sep 22, 2020

View reviewed changes

Comment thread packages/grid/data-grid/rollup.config.js Outdated

oliviertassinari reviewed Sep 22, 2020

View reviewed changes

Comment thread packages/grid/data-grid/rollup.config.js

oliviertassinari requested changes Sep 22, 2020

View reviewed changes

oliviertassinari mentioned this pull request Sep 22, 2020

[docs] Add live demo with DataGrid mui/material-ui#22697

Merged

oliviertassinari mentioned this pull request Sep 22, 2020

[DataGrid] Fix TypeScript definitions published on npm #343

Closed

dtassone requested a review from oliviertassinari September 23, 2020 13:47

oliviertassinari reviewed Sep 23, 2020

View reviewed changes

Comment thread packages/grid/data-grid/src/DataGrid.tsx Outdated

Comment thread packages/grid/rollup.config.js

dtassone added 5 commits September 23, 2020 16:28

fix typings and packages assets

341495b

prettier

afb9f1f

fix types files in pkg.json

856b400

refactor pkgs, remove x-grid-modules

799603d

fix prettier

fix yarn

9a1347d

dtassone force-pushed the fixTypes2 branch from 4e26dcb to 9a1347d Compare September 23, 2020 14:30

dtassone requested a review from oliviertassinari September 23, 2020 14:34

move x-grid-data-generator to packages/ root folder

0ef2c91

oliviertassinari removed their request for review September 23, 2020 14:49

dtassone added 3 commits September 23, 2020 16:56

fix chainproptype import

14a47b4

fix tsconf

0ea25b1

fix karma test

0ac2a37

dtassone requested a review from oliviertassinari September 23, 2020 15:32

oliviertassinari requested changes Sep 23, 2020

View reviewed changes

small refactoring

1f0e926

cleanup root script

6024274

oliviertassinari reviewed Sep 24, 2020

View reviewed changes

Comment thread packages/grid/x-grid/package.json

oliviertassinari approved these changes Sep 24, 2020

View reviewed changes

move x-license to dependencies

a311206

dtassone merged commit 2602f1d into mui:master Sep 24, 2020

	typescript({ tsconfig: 'tsconfig.json' }),
	typescript({ tsconfig: 'tsconfig.build.json' }),

Uh oh!

Uh oh!

Conversation

dtassone commented Sep 22, 2020 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Sep 22, 2020

Uh oh!

Uh oh!

Uh oh!

oliviertassinari commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

oliviertassinari commented Sep 22, 2020

Uh oh!

Uh oh!

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliviertassinari Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtassone Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtassone Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dtassone commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

oliviertassinari commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

dtassone commented Sep 22, 2020 •

edited by oliviertassinari

Loading

oliviertassinari commented Sep 22, 2020 •

edited

Loading

oliviertassinari Sep 23, 2020 •

edited

Loading

oliviertassinari Sep 24, 2020 •

edited

Loading

oliviertassinari Sep 24, 2020 •

edited

Loading

oliviertassinari Sep 24, 2020 •

edited

Loading

dtassone commented Sep 23, 2020 •

edited

Loading

oliviertassinari commented Sep 23, 2020 •

edited

Loading

oliviertassinari commented Sep 24, 2020 •

edited

Loading

oliviertassinari commented Sep 24, 2020 •

edited

Loading

oliviertassinari commented Sep 24, 2020 •

edited

Loading