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

[DataGrid] Fix typings and packages assets #339

Merged
merged 12 commits into from
Sep 24, 2020
Merged

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Sep 22, 2020

The reproduction: typescript-issue.zip.

Closes #343
Closes #38

@dtassone dtassone changed the title fix typings and packages assets [DataGrid] fix typings and packages assets Sep 22, 2020
@dtassone dtassone added the bug 🐛 Something doesn't work label Sep 22, 2020
@oliviertassinari
Copy link
Member

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 oliviertassinari changed the title [DataGrid] fix typings and packages assets [DataGrid] Fix typings and packages assets Sep 22, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 22, 2020

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.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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

packages/grid/data-grid/src/DataGrid.tsx Outdated Show resolved Hide resolved
packages/grid/rollup.config.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari removed their request for review September 23, 2020 14:49
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

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';
                              ~~~~~~~~~~~~~~~~~~~~~~~~

packages/grid/data-grid/src/DataGrid.tsx Show resolved Hide resolved
packages/grid/package.json Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.tsx Show resolved Hide resolved
}),
typescript({ tsconfig: 'tsconfig.build.json' }),
typescript({ tsconfig: 'tsconfig.json' }),
Copy link
Member

@oliviertassinari oliviertassinari Sep 23, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020

Choose a reason for hiding this comment

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

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).

Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020

Choose a reason for hiding this comment

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

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).

.eslintignore Show resolved Hide resolved
packages/grid/package.json Show resolved Hide resolved
packages/grid/package.json Outdated Show resolved Hide resolved
packages/grid/tsconfig.json Outdated Show resolved Hide resolved
@dtassone
Copy link
Member Author

dtassone commented Sep 23, 2020

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
Copy link
Member

oliviertassinari commented Sep 23, 2020

Where did you run this command from?

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

💥

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 24, 2020

I have tried the Lerna release workflow. It doesn't work: https://unpkg.com/browse/@material-next/data-grid@4.0.0-alpha.4/. Tested on my fork.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 24, 2020

#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.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 24, 2020

🎉 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 dtassone merged commit 2602f1d into mui:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Bundling vs dependencies
2 participants