Skip to content
This repository has been archived by the owner on May 29, 2020. It is now read-only.

Build a better foundation for the future #10

Closed
wants to merge 18 commits into from
Closed

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented Apr 20, 2020

Studio was never about a single app. It's about building a platform to enable us to rapidly build internal apps. The proof of concept was easy enough to express via a single Next.js app, but it was already getting messy with barely two apps in it.

Enter this PR.

This lays the foundation for a massive future expansion that can scale to meet our needs. It reorganizes Studio into a monorepo, but it's not your average monorepo. You'll find no lerna or yarn workspaces inside. We never have to link. There's also only one package.json to make sure that all apps run off of consistent versions. It uses Typescript's composite functionality to enable incremental compilation as well as helping automatically map dependencies.

This is really only the beginning. A lot of automation is needed to help with the ergonomics and I've still got to iron out deployments, but I'm super excited about where this project could go next.

I was heavily inspired by a tool called nx. It does some magical things that I'd like to investigate incorporating.

@zephraph zephraph self-assigned this Apr 20, 2020
@damassi
Copy link
Member

damassi commented Apr 20, 2020

@zephraph q:

You'll find no lerna or yarn workspaces inside. We never have to link. There's also only one package.json to make sure that all apps run off of consistent versions. It uses Typescript's composite functionality to enable incremental compilation as well as helping automatically map dependencies.

Would you mind expanding on this a bit? What's a matter with workspaces? That approach is battle tested for scale with docs and an entire package manager built around it. What does this new approach achieve that the other one lacks? Typescript's composite feature is def cool, but it's most commonly used alongside workspaces as a way to consolidate config vs replacing workspaces. Certainly down to experiment with new approaches tho! Just curious for more details.

apps/README.md Outdated Show resolved Hide resolved
libs/README.md Outdated
}
```

It's a bit cumbersome for now, but it's on the roadmap to automate away this requirement.
Copy link
Member

Choose a reason for hiding this comment

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

Does all of this add more complexity than conventional workflows? With workspaces and other such models this kind of paperwork isn't necessary, and automating it away means creating custom tooling for something that isn't necesarily problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the added complexity of lerna + workspaces is way above this. You don't need to learn another tool. You don't need tk think about bootstrap or linking. These references do have to be added, but that's pretty easy to automate.

Copy link
Member

@damassi damassi Apr 20, 2020

Choose a reason for hiding this comment

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

A yarn workspace is pretty much zero-config once a folder name is set in the root package.json. Then folks build things using conventions they're familiar with without much further thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, this is just Typescript and nothing else. It's simple and relatively simple to wrap your head around. If it turns out that it doesn't work out, it's a relatively low lift to add lerna + workspaces. The structure can stay the same, we'd just have to reconfigure how ts compiles to remove composites. If nothing else typescript project references and workspaces can live together. We'd still have to manually (or with automation) configure the references. @orta mentioned there's talk of automatically configuring references in a workspaces/lerna monorepo. If that happens, it's easy enough to move to that.

Given that we can add it without a significant refactor, I say we wait to make that decision. Let me play around with tweaking references. If it doesn't work out, it doesn't work out and we have other viable options.

libs/auth/src/callback.tsx Outdated Show resolved Hide resolved
libs/auth/src/callback.tsx Outdated Show resolved Hide resolved
libs/auth/src/index.ts Outdated Show resolved Hide resolved
libs/utils/src/env.ts Outdated Show resolved Hide resolved
libs/utils/src/string.ts Outdated Show resolved Hide resolved

This directory contains patches for [patch-package](https://github.com/ds300/patch-package#readme)

If you add a new patch you _must_ describe what the patch does and why it's needed in this file.
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @ds300 I'd like to build something to poke people to do this.

@damassi
Copy link
Member

damassi commented Apr 20, 2020

Thinking further, why do we need to imagine divisions between apps that are larger than routes / well-organized folder structures? Next is designed to handle large apps with bundle splitting and so on. It seems like a single package.json and a single tsconfig would be enough? (Thinking, Force, for all of its flaws, is very simple at its core, with dozens upon dozens of apps mounted into a single express app.)

The proof of concept was easy enough to express via a single Next.js app, but it was already getting messy with barely two apps in it.

Is this a problem with Next's design or the work-in-progress nature of an MVP? Would it be worthwhile to propose a more permanent file organization structure that leans into Next's patterns before engineering something more custom?

@zephraph
Copy link
Contributor Author

Thinking further, why do we need to imagine divisions between apps that are larger than routes / well-organized folder structures? Next is designed to handle large apps with bundle splitting and so on. It seems like a single package.json and a single tsconfig would be enough? (Thinking, Force, for all of its flaws, is very simple at its core, with dozens upon dozens of apps mounted into a single express app.)

Force is, in essence, a single application. Sure, it has many sub-express apps but it's representative of artsy.net. Studio is a platform for building apps. That means they can have separate domains (like team nav) and can be deployed independently.

@dblandin
Copy link
Member

I'm still digesting this PR but I have some initial thoughts.

I like the idea of supporting a clearer separation between apps within studio. I like the move towards extracting common patterns (like auth) and utilities into usable imports/packages.

I'm hesitant about introducing a monorepo setup which requires custom automation to retain its simplicity. I'm a bit wary of the new @artsy-studio/next namespace. I'm not sure we need all of that in a shared lib to start. Many of the patterns here will be unfamiliar to those approaching the repository for the first time. Even with automation to hide some of the details, may that additional overhead hurt the overall goal of this repository, to rapidly spin up new internal tools? It may be easy enough to spin up with automation but will it also be easy to understand? Easy to maintain?

If one of the goals of this new architecture is to support separate deployments to different subdomains, can we achieve that with what Next.JS and Zeit offer on their platform, without custom tooling? It seems like "zones" would get us most of the way there: https://nextjs.org/docs/advanced-features/multi-zones

@zephraph
Copy link
Contributor Author

I'm hesitant about introducing a monorepo setup which requires custom automation to retain its simplicity

I understand the concern. All monorepos require automation, whether it's using tools like lerna + yarn workspaces or something like nx. Right now this is only using typescript w/ no other tooling. It's almost monorepo only in name. There are no separate packages, indeed the @artsy-studio/<package> namespace is just an alias to libs/<lib-name>/src/index.ts. It could just as easily be libs. Technically I could reconfigure this to be a single typescript compilation and remove the need for references altogether meaning everything would be automated completely. It'd just compile slower.

This re-organization (which is really the primary change) allows us to use zones whereas previously it was just a single next.js app. It does enable multiple deployments though, which is a nice bonus.

@damassi
Copy link
Member

damassi commented Apr 20, 2020

Out of curiosity, would it be possible to

  • set a yarn workspace in the root package.json that points to apps
  • create a folder called studio and in it set a package.json to @artsy/studio and set it to private
  • create two app folders for whats been created (blog, teamnav), and import @artsy/studio where needed from subapps
  • use the exact same typescript scaffolding, but without needing to set references and live outside of conventional node.js workflows

Echoing Devons concerns, it's a hard sell to push forward on such a custom path that breaks conventions that devs are familiar with. As the architect of the PR the idea makes sense to its author, but having to add references and other bits of overhead for something that should be an yarn install away feels like a question for other devs who quickly want to get to work and not worry about new concepts. @artsy-studio being an alias to a folder lib also feels weird. All of our libs use @artsy (questions from other devs). The TS team may support workspaces in the future, but that's just a may. Right now things feel a bit premature to step outside a project organization pattern that is tried and true.

But really my main concern is why we're thinking about this kind of scale right now where a good old folder structure brainstorm might suffice. Are current typescript compile times that slow? If so, is that an indication of some other problem? In reaction, with incremental type-checking hitting thousands of files, its just a sec or two. It works perfect without the need to split anything apart or add more mental overhead.

I would vote for a MVP that sets up a version of of @artsy/studio with a working implementation of Next's zones, then see what's needed to maintain a bare-bones minimalism without any workspace concept whatsoever. If the concern is about things getting messy with just two apps, it might be worthwhile to ask for folks to review and suggest improvements.

@zephraph
Copy link
Contributor Author

Thanks for the feedback all. I'm going to put this back into draft and think about it some more.

@zephraph zephraph marked this pull request as draft April 20, 2020 20:07
@orta
Copy link

orta commented Apr 20, 2020

FWIW on the TypeScript website mono-repo, I don't use composite projects but two small bits of automation to ensure everything is consistent across many modules:

This is enough to ensure all the internal modules are correctly built and referenced throughout the repo and then it works via vanilla node module resolution

@zephraph
Copy link
Contributor Author

Thanks @orta 👍

Given the feedback on complexity concerns, I've tried to simplify and
clarify where I could.

- The TS build no longer requires manual configuration
- The @artsy-studio nominclature for importing from libs has been removed
- libs is now just a directory that holds code. Any app can import from it normally.
- There's a generator to quickly spin up apps. It's pretty cookie cutter for all apps.
@zephraph
Copy link
Contributor Author

zephraph commented May 4, 2020

I'm abandoning this PR. It was an interesting thought experiment, but multi-zone deployments aren't actually supported by Zeit (despite their example) and it's just getting more complicated.

I've extracted out team nav to another project and I'm going to simplify the scope of this overall project and create a tech plan to get buy in from other folks before doing any more work on it.

@zephraph zephraph closed this May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants