artsy / README

:wave: - The documentation for being an Artsy Engineer
Creative Commons Attribution 4.0 International
1.1k stars 120 forks source link

[RFC] Move palette iOS component implementations to the eigen repo #318

Closed ds300 closed 4 years ago

ds300 commented 4 years ago

What

This RFC proposes that we move the implementations of the iOS versions of palette's 'Elements' + 'Components' from the palette repo to the eigen repo.

'Tokens' (i.e. icons, colors, spacing, and typography config) could remain shared between platforms.

Why

We originally kept the iOS + web components in the same place for good reasons. Two years later I think we can revisit some of those reasons:

  1. "It lets components share implementation details"

    In practice most component implementations do not share code, or are made considerably more complicated in the attempt to make them share code (e.g. Button, Avatar).

  2. "It lets us keep the API consistent across platforms"

    This is a nice plus, but often exact consistency is problematic.

    • The base layout/text elements are different on each platform. e.g. at the moment Flex is missing commonly-used props that the base react-native View component has, like onLayout, and testID. We could add these props to the palette component but then they'd be visible on web too, which is not ideal.

    • User interaction works differently between platforms and sometimes that means our own components' props needs to be different, e.g. the Select on web is just a normal dropdown, but for iOS it needs to optionally show a search box with autocomplete, which requires extra data.

  3. "The components can be re-used in other projects"

    This has been true for the web, but we do not have plans to create another react-native app. We might one day, but it doesn't make sense to spend effort optimising for a hypothetical future when the risk:reward ratio involved in ignoring that hypothetical future is so favorable.

"How favorable", you ask? Here's a number of compelling reasons to adopt this change:

  1. We could avoid web-only updates to palette which cause CI failures in eigen, requiring extra PRs/commits to fix

    e.g https://github.com/artsy/eigen/pull/3396 https://github.com/artsy/eigen/pull/3362 https://github.com/artsy/eigen/pull/3340 https://github.com/artsy/eigen/pull/3390 https://github.com/artsy/eigen/pull/3318 https://github.com/artsy/eigen/pull/3489 https://github.com/artsy/eigen/pull/3396 https://github.com/artsy/eigen/pull/3458

  2. We could leverage RN-specific tooling for palette components

    e.g. in eigen we have a custom lint rule to prevent text strings from being rendered outside of a Text component, since react-native throws a hard error when that happens. We also have test configuration that is particular to react-native. It makes sense to leverage this tooling when working on iOS components, but it would be a pain to have to maintain the tooling in both the eigen repo and the palette repo.

  3. We could avoid web-specific tooling that provides a subpar DX in react-native

    e.g. styled-components's css template string syntax allows people to use features of css that don't exist in react-native, and it will either fail silently or with cryptic error messages which lack stack traces.

  4. Ease of development for eigen devs

    Linking palette to eigen seems to be a common source of pain, and no doubt we all know the costs associated with having to coordinate related PRs accross multiple repos.

    Also, debugging local components is way easier since you are tracing untranspiled code.

  5. Ease of maintenance

    Major changes (e.g. styled-system upgrades) could be applied to each platform independently without blocking the other from receiving updates to palette in the meantime.

    We'd also simplify the palette repo itself, e.g. It wouldn't have to run its test suite twice and we could remove the architectural gymnastics involved in sharing code between platforms.

How

In broad strokes, we'd copy the palette source code to eigen, then in both palette and eigen we'd delete anything we can delete and simplify anything we can simply.

In eigen specifically we'd keep palette in its own folder hierarchy to keep it distinct for exploratory reasons. We could also look into adding a pattern library, like palette.artsy.net, to eigen's admin menu.

I'd suggest that we keep sharing the palette 'tokens' (i.e. icons, colors, spacing, and typography config), since they don't change often and they're easy enough to share between platforms. In an ideal world they'd live in a separate package in the palette monorepo so that eigen can depend on them directly rather than continuing to depend on the @artsy/palette package. That might be more trouble than it's worth in terms of CI config though.

I'm very happy to undertake all of the above work.

Additional thoughts

I hope that this whole idea avoids coming across as isolationist. Palette is wonderful tool to help Artsy engineers and designers be more productive, both individually and in collaboration. This RFC shouldn't change that in any way. Palette's implementation is already widely distributed across Figma, Notion, and artsy/palette. Even artsy/eigen, with iOS versions of ReadMore, Select, Image, and Input all happily living there already! Let's make it a wider happy eigen palette family?

dzucconi commented 4 years ago

I very much support this. Tokens do most of the footwork in maintaining consistency between platforms and so extracting those into their own package makes a ton of sense. After that implementation and UX differ in large ways.

One additional point here is that without tree-shaking we're also shipping twice the amount of code to force and other web consumers of palette.

damassi commented 4 years ago

Our universal component implementation got us pretty far and worked surprisingly well for a while, but there have been some serious 🤔 moments along the way that could have been eliminated by keeping the platforms separate. There's also a lot more momentum on the RN side these days where additional problems are surfacing.

It will also be really nice to see that "missing react-native" peerDep warning go away on web 😄

👍

ashfurrow commented 4 years ago

I'm not going to lie, my initial reaction was less-than-enthusiastic. But you've laid out a really convincing argument, and have convinced me. That list of failed Palette update PRs to Eigen, oof. The tradeoffs aren't really worth it anymore; as Chris said, our implementation has gotten us quite far but it might be time to go another route. So I'm a 👍 here.

admbtlr commented 4 years ago

I'm guessing that the imminent Android version is also going to be a factor here. I don't know how you're thinking about architecting it, or whether it will be in the Eigen repo (I'm assuming it will?), but I can imagine that it will add another layer of complexity to the interplay with the separate Palette repo.

ds300 commented 4 years ago

That's a good point. If we don't make this change we'd have to rename most .ios.ts(x) files in palette to .native.ts(x) to allow them to be picked up when making the android bundle. There's probably other complications too.

And for the record, yes we're planning to keep android and ios versions of the app in the eigen repo, set up like a normal react-native app as much as possible. The technical plan is outlined here: https://www.notion.so/artsy/Supporting-Android-7cc6310179334a48bad64c58caf726f8

admbtlr commented 4 years ago

Ah, great, thanks for the link to the technical plan!

mdole commented 4 years ago

Excellent points! I'm for it. One transition question: would it be worth keeping the RN components pulled out of Palette in a package (i.e. palette-react-native or something)?

Advantages:

Disadvantages:

ds300 commented 4 years ago

I like the idea of sharing the love but it would be a fair amount of extra up-front work, and we'd lose some of the major advantages I outlined above. So i feel like it makes sense to start out by prioritising everyday bread-and-butter development rather than once-in-a-blue-moon experiments

mdole commented 4 years ago

Yep sure! Sounds like a plan to me

pvinis commented 4 years ago

I think that makes sense, it will simplify they dev work on palette. I have one concern and one idea.

My concern is how do we keep the same "language", like Tag or Card across the in-eigen palette and the regular palette? I don't think this is particularly hard, but I think it's worth bringing up.

My idea is that after we have in-eigen palette, it would be easy to add storybook again. I remember that it was removed because of the palette and eigen connection causing difficulties, and I'd be happy to try adding it back after the merge.

ds300 commented 4 years ago

My concern is how do we keep the same "language", like Tag or Card across the in-eigen palette and the regular palette? I don't think this is particularly hard, but I think it's worth bringing up.

The naming scheme should follow the palette specs in figma. If we accidentally create naming inconsistency between the platforms it's no biggie to fix it on an ad-hoc basis.

My idea is that after we have in-eigen palette, it would be easy to add storybook again. I remember that it was removed because of the palette and eigen connection causing difficulties, and I'd be happy to try adding it back after the merge.

:+1: We removed storybook because the dev workflow it had originally been set up for (back in the emission days) was problematic, and nobody was using it. It could be cool to add it back as a kind of component library, like palatte.artsy.net.

zephraph commented 4 years ago

I'm actually 👍 on this as well.

A lesson we learned early on is that creating everything completely universal isn't feasible. You have to bend over backwards to make that work. There were on-going issues with prop pollution that took some pretty conscious effort to avoid. Generally there's a trap of thinking web first too, which has lead (in some cases) to components getting built that looked like they'd be available on RN, but wouldn't actually work.

More to the point, I'd noticed a trend of building components that likely would be suited for palette directly in Eigen without major visible effort to backport them. That was notable to me that something was breaking down and our current approach is insufficient. I considered advocating that they be moved, but I ultimately came to a very similar position... the value vs effort of moving those is quite limited (at best).

It'd be one thing if we had many different mobile products, but even in that case @mdole's previous suggestion would likely be a better answer.


The one thing that we do need to account for is a way to track coverage of what components exists across each platform. That's already a need that we had, but this just makes it doubly so. I'm not necessary sure exactly where that should be tracked though. We can work with design on that part.

pvinis commented 4 years ago

The one thing that we do need to account for is a way to track coverage of what components exists across each platform. That's already a need that we had, but this just makes it doubly so. I'm not necessary sure exactly where that should be tracked though. We can work with design on that part.

☝️☝️☝️

ds300 commented 4 years ago

I think the notion page would be a sensible place to track that for now. I'll plan to create and fill in a table as part of the resolution for this RFC.

ds300 commented 4 years ago

Resolution

We decided to do it.

Level of Support

2: Positive feedback.

Additional Context:

Everybody was in favor of the idea. Some suggestions and caveats were raised and addressed.

Next Steps

I'll create an MX ticket for the following tasks:

  1. Spike on moving design tokens out into separate package in the palette repo
  2. Copy iOS component implementations from palette repo to eigen repo.
  3. Refactor palette package to be web-only.
  4. Create table on notion documenting component implementation statuses

Exceptions

If/When we create another RN-based app we can look at moving the component implementations back out into a separate RN-focused library.

ds300 commented 4 years ago

jira ticket https://artsyproduct.atlassian.net/browse/MX-441