Skip to content

[core] Proof of concept for solving the .d.ts generation issue #24132

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

Closed
wants to merge 1 commit into from

Conversation

oliviertassinari
Copy link
Member

One part of #24112.

@oliviertassinari oliviertassinari added typescript core Infrastructure work going on behind the scenes labels Dec 26, 2020
@oliviertassinari oliviertassinari changed the title [core] Proof of concept for soving the .d.ts generation issue [core] Proof of concept for solving the .d.ts generation issue Dec 26, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Dec 26, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 919a160

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Let me take a look at that. This is only fixing symptoms but not the underlying issue.

@eps1lon eps1lon self-assigned this Dec 27, 2020
@oliviertassinari oliviertassinari marked this pull request as draft December 27, 2020 14:31
@HofmannZ
Copy link

@eps1lon Anything we can help with?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 28, 2020

@eps1lon Agree, let's hold on for some time. I have tried to find the root issue but gave up after an hour. If we can figure it out, that would be better, in case we hit the problem in another case in the future.

If we don't, I think that we could live without it too. The proposed approach 1. wrecks the types with withStyles, but we are moving to styled anyway. 2. forces us to manually type the classes which seems to help with the migration to styled.

@HofmannZ During the end of year holidays, things might move slowly. Feel free to spend time exploring what's wrong. The closest I could find was microsoft/TypeScript#24599 which was relative to a fs case sensibility issue in the core of TypeScript.

@eps1lon
Copy link
Member

eps1lon commented Dec 28, 2020

@eps1lon Anything we can help with?

My intuition tells me we need a change at the tsconfig/tsc level. The current approach doesn't guarantee that it doesn't regress in the future.

Alternatively we get a smoke test for the build output (started in #23166) that catches these regressions.

Either way I want to make sure this doesn't get lost over the holidays since it's important to get right before v5. I rather block it in this instance to keep visibility up instead of merging a quick fix that'll just re-surface later.

@eps1lon
Copy link
Member

eps1lon commented Dec 28, 2020

The generated declaration from tsc is just so broken in this instance:

  1. inlines type imports
  2. uses path imports relative to root dir instead of built file layout
  3. removes WithStyles<typeof styles> intersection

It makes sense because it's pretty hard to generate a sensible type in this particular instance but tsc should throw instead of garbling this particular declaration file.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 28, 2020
@oliviertassinari oliviertassinari deleted the 24112 branch January 4, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants