[Toolchain] Add emission-related package.json scripts #109

damassi · 2018-10-03T22:33:30Z

Adds a few new cli commands to make working in Emission a bit easier in the absence being able to yarn link changes between modules:

yarn compile:emission
yarn watch:emission

The flow when working is to start emission and then run, in a second pane, yarn watch:emission which will then start the emitDeclaration + compile process. Changes are reflected instantly:

(Note: This presumes that an /emission folder exists alongside /palette on the developer's HD)

artsyit · 2018-10-03T22:35:16Z

Deploy preview for artsy-palette ready!

Built with commit b99aeba

https://deploy-preview-109--artsy-palette.netlify.com

damassi · 2018-10-04T07:21:21Z

src/elements/Box/Box.tsx

@@ -56,7 +57,9 @@ export interface BoxProps
 * Box is just a `View` or `div` (depending on the platform) with common styled-systems
 * hooks.
 */
-export const Box = primitives.View.attrs<BoxProps>({})`
+export const Box: StyledComponentClass<any, any, any> = primitives.View.attrs<


@alloy, @orta :

Spent a long long time debugging why we were getting type issues in Emission, but this seems to fix it (though in unsatisfactory way).

Some background:

We started using universal components such as <Flex> and <Spacer> in Emission in 2.11.0, which is where emission currently is at, and is the last version that type-checks properly. Around this time I started organizing palette and moving stuff over from Reaction, and then it started to pick up steam with the rest of the team and ton of additions have been made since then. Looping back today I wanted to make sure that our universal implementation still worked and found all kinds of errors related to the styled components output changing in the emitted .d.ts files.

In 2.11.0, this is the output:

export declare const Box: import("styled-components").StyledComponentClass<any, any, any>;

However, somewhere along the way, and after a few dependency changes, this is now the output:

export declare const Box: StyledComponentClass<Pick<{}, never> & Pick<ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & FlexProps, "alignContent" | "alignItems" | ... and the rest of the dom ... >>;

Thinking further, now that these types have been filled out on the styled-components.d.ts side of things (maybe since they moved to @types/styled-components, which we're now using in palette?) I'm not sure how we could get this to properly type-check given the fact that

import { styled as primitives } from "../../platform/primitives" ... export const Box = primitives.View` ... `

is platform-dependent code -- meaning that when RN encounters this import, it would see .ios in the filename (platform/primitives -> platform/primitives.ios.tsx) and conditionally import the correct file (and ideally the correct types); however, with tsc we can't set up conditional paths like this, and so from Emission it sees the platform/primitives import and thus fails since we're typechecking against web specific code rather than the correct .ios.tsx counterpart.

I'm at a loss as to how to address this in a future-proof sort of way. (I could also very well have found myself in lost in a maze and missing something somewhere.)

If you pull down this branch and remove these any's from the following code its easy to reproduce the error from emission after running yarn watch:emission from here, waiting till its done compiling, and then starting yarn type-check from the emission folder.

Yep - this is definitely something I've been wondering about.

Digging around, microsoft/TypeScript#8328 seems like the best description on the problems, and microsoft/TypeScript#21926 is the best open request for path resolving.

I wonder if it makes sense for us to move Emission (and Palette?) to use a TSC fork that includes this commit: acoates-ms/TypeScript@f023961

For palette checking we can have two tsconfigs - one for RN with extensions, and and for web as vanilla, so CI would compile twice.

We should have separate TS configs for platforms anyways, because e.g. we can’t have @types/node in a RN project and various other web only typings, nor can we have @types/react-native for web projects.

So what if we just decide to always add the platform extension when a component needs platform specific implementations? i.e. MahComponent.web.tsx and MahComponent.ios.tsx. That way we can just add those patterns to the TS config exclude patterns. i.e. "exclude": ["src/**/*.ios.*"] and "exclude": ["src/**/*.web.*"]

Ah but wait, that still wouldn’t fix platform specific imports 🤔

cc @ds300 - adding David to the thread since he has a lot of experience in this domain!

This might be a good topic to present at Show & Tell, to get more eyes on the problem 👍

damassi · 2018-11-06T20:52:05Z

src/elements/Box/Box.tsx

-export const Box: StyledComponentClass<any, any, any> = primitives.View.attrs<
-  BoxProps
->({})`
+export const Box = primitives.View.attrs<BoxProps>({})`


Does

export const Box = primitives.View<BoxProps>` ... `

work?

damassi · 2018-11-06T20:52:46Z

src/elements/Flex/Flex.tsx

-export const Flex: StyledComponentClass<any, any, any> = primitives.View.attrs<
-  FlexProps
->({})`
+export const Flex = primitives.View.attrs<FlexProps>({})`


Same here (would you mind updating other definitions too while you're at it?)

javamonn · 2018-11-06T21:30:50Z

I've pushed a commit that removes the any cast and adds some missing ios stub components. I don't understand what changed, but note that palette currently typechecks in emission: see yarn ci passing in artsy/emission#1215. The stub components added here are needed as otherwise emission throws runtime errors.

This works for me locally but I'd appreciate others sanity-checking this as it does seem strange to me - I'd appreciate if someone involved in earlier discussions (@damassi, @orta , @l2succes, @alloy) could take a look?

To test this out, from this branch:

Compile palette, copy to emission's node_modules: yarn compile:emission
From emission, it should type check: yarn type-check
2a. It'll actually fail type-check as-is, but you should be able to remove the missing alignSelf prop to resolve the error: https://github.com/artsy/emission/pull/1215/files#diff-08bdd43785cf0f033df1fee3ee69d0beL14
From emission, it should run without errors: yarn start

damassi · 2018-11-06T21:36:16Z

src/elements/Tooltip/Tooltip.tsx

 }

-const Tip = styled(BorderBox)`
+const Tip = styled(BorderBox)<TipProps>`


@javamonn - right on, ty for updating everything 👍

javamonn · 2018-11-06T21:38:51Z

To expand, the component types here are non-ideal and DOM-centric as TS doesn't pick up the platform specific implementation:

export declare const Box: import("styled-components").StyledComponentClass<React.ClassAttributes<HTMLDivElement> & React.HTMLAttributes<HTMLDivElement> & BoxProps, any, React.ClassAttributes<HTMLDivElement> & React.HTMLAttributes<HTMLDivElement> & BoxProps>;

This is something that could still be improved in the future per the discussion above, https://github.com/artsy/palette/pull/109#pullrequestreview-161474475. What's strange to me is why this type-checks in Emission now when it didn't when this PR was originally opened.

These types are also a potential footgun in context of emission, as I think TS would let you pass a DOM specific prop (e.g. className), and that would blow up at runtime? But regardless, it's better than any casting.

damassi · 2018-11-06T22:36:41Z

@javamonn

¯_(ツ)_/¯

damassi · 2018-11-06T22:40:52Z

Oh! Update:

When running

palette $ yarn watch:emission
emission $ yarn type-check-w

It produces this error:

(I'm certain that overflow: hidden is supported but that seem tangential)

javamonn · 2018-11-07T15:25:16Z

@damassi Ah, I see these as well. These both look like minor fixable errors, the second one is expected and requires a change on emission, which we can do as part of upgrading once this PR is merged.

The first one I think I understand what's going on, but not sure how to fix: BorderBox extends Flex, but FlexProps doesn't include props from the underlying primitives.View, so the consumer (Emission in this case) doesn't know that a style prop would be passed down to the primitive view. Is there a way we could include the underlying primitive props - I think this is ReactNative.ViewProperties? Though, because platform specific types don't work yet, we would want to include something like React.DetailedHTMLProps<React.HTMLAttributes<HTMLDivElement>, HTMLDivElement> I think.

Perhaps the underlying primitive prop types could be exported from primitives, or is there a way we can get the full prop types from a component? In the case of BorderBox -> Flex, could we unwrap Flex from:

const Flex: StyledComponentClass<ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & FlexProps, any, ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & FlexProps>

to the prop type: ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & FlexProps ...

Thanks for taking a look!

ashfurrow

Nice! Thanks for cleaning up those generic types, too 💯

javamonn · 2018-11-19T21:48:34Z

@l2succes and I paired on this for a bit and the first error noted by @damassi above was resolved by fixing a typo in the type parameterization on BorderBox. The other error (as well as one additional recently introduced error) have fixes over in Emission: artsy/emission#1215.

This PR should be good to merge.

As a follow up, we'll work on figuring out how to ship RN palette types to Emission instead the current DOM based types as noted here.

artsyit · 2018-11-20T16:45:42Z

🎉 This PR is included in version 2.21.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

damassi assigned orta Oct 3, 2018

damassi force-pushed the add-emission-scripts branch from 46ff57d to 4e02622 Compare October 4, 2018 06:57

damassi commented Oct 4, 2018

View reviewed changes

damassi commented Nov 6, 2018

View reviewed changes

ashfurrow approved these changes Nov 9, 2018

View reviewed changes

javamonn force-pushed the add-emission-scripts branch from ddfee3c to fbf922c Compare November 13, 2018 20:23

[Toolchain] Add emission package.json scripts

a5a2eea

javamonn force-pushed the add-emission-scripts branch from fbf922c to 299fce4 Compare November 19, 2018 19:13

[FIX tooling] Remove any casts, add ios missing ios stub components

b99aeba

javamonn force-pushed the add-emission-scripts branch from 972e572 to b99aeba Compare November 19, 2018 21:42

javamonn assigned l2succes Nov 20, 2018

l2succes merged commit 16e3ed9 into artsy:master Nov 20, 2018

This was referenced Nov 28, 2018

[Tooling] Improve Emission scripts to support testing of dependency updates #136

Open

[FEATURE ios] Add Collapse implementation #135

Merged

[Tooling] Fix React Native types #137

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Toolchain] Add emission-related package.json scripts #109

[Toolchain] Add emission-related package.json scripts #109

damassi commented Oct 3, 2018 •

edited

Loading

artsyit commented Oct 3, 2018 •

edited

Loading

damassi Oct 4, 2018 •

edited

Loading

orta Oct 4, 2018

alloy Oct 4, 2018 •

edited

Loading

alloy Oct 4, 2018

damassi Oct 4, 2018

ashfurrow Oct 16, 2018

damassi Nov 6, 2018

zephraph Nov 19, 2018

damassi Nov 6, 2018

javamonn commented Nov 6, 2018 •

edited

Loading

damassi Nov 6, 2018

javamonn commented Nov 6, 2018

damassi commented Nov 6, 2018

damassi commented Nov 6, 2018 •

edited

Loading

javamonn commented Nov 7, 2018

ashfurrow left a comment

javamonn commented Nov 19, 2018

artsyit commented Nov 20, 2018

[Toolchain] Add emission-related package.json scripts #109

[Toolchain] Add emission-related package.json scripts #109

Conversation

damassi commented Oct 3, 2018 • edited Loading

artsyit commented Oct 3, 2018 • edited Loading

damassi Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alloy Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javamonn commented Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

javamonn commented Nov 6, 2018

damassi commented Nov 6, 2018

damassi commented Nov 6, 2018 • edited Loading

javamonn commented Nov 7, 2018

ashfurrow left a comment

Choose a reason for hiding this comment

javamonn commented Nov 19, 2018

artsyit commented Nov 20, 2018

damassi commented Oct 3, 2018 •

edited

Loading

artsyit commented Oct 3, 2018 •

edited

Loading

damassi Oct 4, 2018 •

edited

Loading

alloy Oct 4, 2018 •

edited

Loading

javamonn commented Nov 6, 2018 •

edited

Loading

damassi commented Nov 6, 2018 •

edited

Loading