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

Ensure material UI regression is fixed #1607

Conversation

JakeGinnivan
Copy link
Contributor

Fixes #1167

@changeset-bot
Copy link

changeset-bot bot commented Nov 5, 2019

💥 No Changeset

Latest commit: eb747f6

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@JakeGinnivan
Copy link
Contributor Author

Up to you if you want to merge this test

@Andarist
Copy link
Member

Andarist commented Nov 7, 2019

I will gladly merge this - although I would prefer moving this test to a separate file. When we add more stuff at the bottom it will be hard to see where this test actually starts and where it ends.

Additionally, I see a problem on CI with older versions of TS not recognizing builtin Omit (rightfully so). Is it possible to limit TS versions being tested on per test file basis? I had problems with this recently in #1575 . If it's not possible then just inline Omit into this file.

@JakeGinnivan
Copy link
Contributor Author

Will sort out over next few days.

@JakeGinnivan JakeGinnivan force-pushed the chore/verify-fix-of-material-ui-regression branch from 67d64c3 to 0e17373 Compare November 11, 2019 00:53
@codecov
Copy link

codecov bot commented Nov 11, 2019

Codecov Report

Merging #1607 into next will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted Files Coverage Δ
packages/babel-plugin-emotion/src/index.js 94.44% <0%> (ø) ⬆️
...tion/src/utils/transform-expression-with-styles.js 100% <0%> (ø) ⬆️
scripts/babel-tester/src/index.js 85.71% <0%> (ø) ⬆️
scripts/old-babel-tester/src/index.js 100% <0%> (ø)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 11, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit eb747f6:

Sandbox Source
Emotion Configuration

@JakeGinnivan JakeGinnivan force-pushed the chore/verify-fix-of-material-ui-regression branch from 0e17373 to eb747f6 Compare November 11, 2019 01:30
@JakeGinnivan
Copy link
Contributor Author

Not sure what is going on with this build error =/ It's happening locally but I can't see why

@Andarist
Copy link
Member

Seems like it has problem extracting TypographyProps. Not sure if this is worth fighting if it only affects "older" versions of TS. OTOH the situation here doesn't seem overly convoluted so maybe it's worth investigating why this fails - I'd suggest trying to bisect that test to see when it starts to fail. If we'd know that we could maybe assess how many use cases this might affect.

We could also try to only test this with newer versions of TS but I'm not sure how exactly this can be achieved on per-file basis. I've found such comment in dtslint repo - https://github.com/microsoft/dtslint/blob/971f5e2f90285646c97cb7b4a2b1b8b5a555cfdc/src/util.ts#L101 . So seems like it's possible to run against some version, but in here we'd ideally want to run those for all versions since a particular version 🤔

If I find time today for this - I'll try bisecting as well.

@JakeGinnivan
Copy link
Contributor Author

I tried adding the comment, it didn't do much :(

It does work inside the .d.ts files, but not the test files. Not sure it's worth it?

@Andarist
Copy link
Member

Andarist commented Dec 7, 2019

Because it seems to be hard to set up a test for this I don't think there is much point in keeping this open now. The good thing is that we got a confirmation that the issue got fixed.

@Andarist Andarist closed this Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants