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

Fix macro with ts #2427

Merged
merged 2 commits into from May 29, 2019

Conversation

Projects
None yet
4 participants
@mAAdhaTTah
Copy link
Contributor

commented Mar 5, 2019

Fixes #2378. That issue will mistakenly closed, as the issue lives in the macro, not in the types. I figured providing a solution would help clarify the issue.

I also added a test for multiple imports, as I didn't expect to see them split in the test I wrote, so I wanted to confirm and test that behavior before I added my change.

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Bumping this, as it's preventing us from upgrading to the latest version of react-scripts (it bumps the version of babel-macro, which surfaced this issue). Happy to do whatever is needed to land this.

Thanks!

@Igorbek

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I don't really get how it relates to TypeScript. Does it?

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@Igorbek The test is the best explanation:

import styled, { DefaultTheme } from '../../macro'

This will throw a MacroError because of the includes check I removed. This makes it impossible to import TypeScript types from styled-components/macro because those types don't actually exist in the package.

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

The issue linked above (#2378) has more context, if that helps.

@lucleray

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@mAAdhaTTah Have you tried importing DefaultTheme from styled-components instead of styled-components/macro ?

IMO it doesn't really makes sense to import a type from a macro (maybe @kentcdodds has more insights on this?).

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@lucleray I discussed with @kentcdodds on the babel-macro repo; his thought was you should actually whitelist the TS types. I figure that's probably a step too far, since you don't maintain the TS types, so I just removed the validation.

Have you tried importing DefaultTheme from styled-components instead of styled-components/macro?

This would work, but then requires a separate import for SC imports vs types. Also means I can't add a lint rule that says "only import from styled-components/macro", and I have to remember when I should be importing from the macro vs the root package.

It's minor overhead, but for what benefit? The macro doesn't need the imports to be valid in order to do its thing, so I'm not sure why we should keep it and push that off to the consumers.

@lucleray

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@mAAdhaTTah You don't want your application to break if the order in which typescript and babel-macros are run in CRA change and the best way to do that is to import the types from styles-components.

If you start importing the types from the macro and typescript is run before babel-macro, you'll get an error.

IMO it still makes sense to show an error here, because it's not completely equivalent to import from the macro and from sc.

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

If you start importing the types from the macro and typescript is run before babel-macro, you'll get an error.

This is only true with the check in place.

IMO it still makes sense to show an error here, because it's not completely equivalent to import from the macro and from sc.

Why not? Validating whether the imports are valid doesn't seem like the responsibility of the babel plugin so much as the bundler (whether that's webpack/rollup/etc. or the browser), and given that the types for the macro are re-exports of the root types, they should be entirely equivalent, except for the fact that the macro throws when encountering type imports. Without the check, the macro works fine, so the validation doesn't help avoid issues with the macro being unable to run over a file, so I'm not sure what value it's providing besides requiring me to split my type imports from my macro imports.

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Sorry to be a pain, but bumping this again.

I think I'd like to suggest thinking about it from this perspective: Setting aside TypeScript at all here, what benefit does validating the imports in the macro provide to either the consumers or maintainers of styled-components?

@lucleray
Copy link
Member

left a comment

Looks good to me. After reading @mAAdhaTTah's comments, I think it won't break anything to merge this and it might make things easier for some people.

Thanks @mAAdhaTTah for your patience 🙏

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@probablyup Last poke, I swear, but we've got 2 approvals, can we get this merged & released? Thanks!

@probablyup

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@mAAdhaTTah yup just need you to add a changelog entry

@probablyup probablyup added the 4.0 label May 29, 2019

@mAAdhaTTah mAAdhaTTah dismissed stale reviews from Igorbek and lucleray via cd1e021 May 29, 2019

@mAAdhaTTah mAAdhaTTah force-pushed the stellaai:fix-macro-with-ts branch from f302f58 to cd1e021 May 29, 2019

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@probablyup Done!

mAAdhaTTah added some commits Mar 5, 2019

Add test for multiple imports
Check that the imports are split up normally.

@mAAdhaTTah mAAdhaTTah force-pushed the stellaai:fix-macro-with-ts branch from cd1e021 to a53b839 May 29, 2019

@probablyup
Copy link
Contributor

left a comment

Thanks!

@probablyup probablyup merged commit 28f561d into styled-components:master May 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@probablyup

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Could you possibly make a second PR against canary when you get a chance?

@probablyup probablyup referenced this pull request May 30, 2019

Merged

v4.2.1 #2574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.