I find "unique" a strange way here, it reads as some claim of novelty. Maybe "custom" or "its own"? Also I do find the addition of "to this repo" somewhat redundant (even if more precise)

That's fair. I can simplify this a bit.

Thought: why.md isn't a very descriptive name and this focuses on a specific aspect of the architecture. Though it does seem relevant and useful as such.

Here's one thought: Merge what.md, why.md, and the Structure section of how.md into architecture.md , and this could be a ## Motivation section there?

That would make sense to me. The historical context for how things used to work, and the specific problems we have, will rapidly become less relevant. Explaining the ongoing benefits is useful though.

Reminds me of the old README this package had, though that was far longer and even less relevant. There was an elaborate justification given for why we should use TypeScript over JavaScript. I'm sure people needed convincing at the time, but it felt a bit misplaced after we became more invested in using TS as a team.

Good points! I'll try condensing the content and removing the historical angle. (edit: or at least rewording it to sound like it comes from the present and not from the past)

I think we can assume the reader knows how to get a GH access token (or look it up elsewhere)? Emphasis on classic seems relevant, though.

The linked article is the official documentation. That does seem like the best place to point people. I guess we could shorten this sentence to omit the "go into the token settings" part, as that's already covered by the article as well.

Something like this?

alternatively,

We should probably make it normative that people run CI on their own forks? (As-is this not explicit about it, considering "this repo" technically depends on if you read it before or after forking). Consider that this will also be perused by people unaffiliated with the MetaMask team.
Would there be additional setup instructions (like here we should mention or link to to capture that?

This trigger only works for branches on this repo. Forks aren't supported by preview builds. We have been encouraging people on the team not to use forks, because compatibility with certain CI steps is worse.

It's difficult to safely build any integrations that work with forks and allow interacting with the upstream repo. We don't want to give untrusted third parties access to a write token for this repository for example, even one scoped just to comments, because that would let them create fake metamaskbot comments.

I guess I have to look into preview builds and try it out to see how the limitations arise.

Some of the below is just opinion, even if I write normatively.

Without having seen more than 1 case of things behaving differently, it seems like when it comes to producing builds and uploading them, there is fundamentally nothing apart from setting different registry and access token etc that should differ...? Ie everything done in this central repo should be doable transparently on forks as long as the expected env vars / secrets are there.

This is also with the assumption that we want to allow contributors on equal terms even if they're not part of the MM org. Even if we'd say and think we do, failing CI steps, as well as documentation presented as general but effectively only relevant for MM org internal use (I believe that first-time contributors need manual approval to have their draft PRs builds, and that having a draft PR pop up here for everyone who want to make a build is going to become untenable sooner than later) make that less likely and affect expectations, reducing probability of potential contributors actually engaging constructively.

Consider also overage costs and throughput for CI - things add up.

I completely understand that documentation should reflect reality. But I also would like to accommodate and normalize a more distributed flow. Perhaps this file/section can stay on the cooler (that is, pull it out of this PR for now to not block) until the outstanding CI issues are resolved and we can present something that can also set best practices and be more generally applicable?

Until then (given some of the complications above), and without clearer enumeration of alternatives (talked about below), I'd consider this internal docs rather than something that's useful to make public.

We absolutely want to ensure external contributors can be effective contributors. It should be straightforward enough to document how they can manually publish to their own private registry to test changes.

The only part of "preview builds" we can't allow from forks is the use of our registry, and the ability to trigger publishing from comments on our PRs. But neither of those are essential to allow contributors to publish and test changes.

I don't see a problem with including internal docs as public documentation in the README, as long as it's clearly documented as internal. Ideally we'd have equivalent steps for external contributors though, where possible/relevant.

I feel like we should still include a part about preview builds in the docs as well. This is a new thing for all engineers, so featuring it in the docs raises visibility to those unaware and keeps us from having to explain how it works.

I can work on documenting how this could work for contributors.

One question I have here as I rewrite this is, why are we requiring the @metamask-bot comment to only work on draft PRs? I feel like it complicates things a bit because I have to include this bit:

Continue to keep your controllers PR in draft mode until you're sure that you don't need to make any new changes. If you do push up a new commit to your PR, however, simply post another @metamask-bot preview-build comment and this will trigger another build.

I don't think we are doing that. We used the preview-builds command for the v34 release PR well after bringing it out of draft.

We're requiring extension and mobile PRs that utilize these builds to be draft PRs, so that we don't accidentally merge preview builds into the main branch.

This looks like it's intended to or used to be a numbered list?

Oops! Not sure why I didn't catch this 😬

I know it's already prevalent but IMO we (see what I did there) should avoid speaking from first-person in docs like this - reserve that for discussion and P2P comms.

I can probably guess the reasoning for this, so I don't disagree, but I'm just curious — why in your opinion should this be avoided?

There is more to it but here is one aspect: By introducing the "we", the reader is now forced to consider the author explicitly; who is writing this, when? In case of some open-source projects: Is this a message from the original author, the intermediary repository maintainers, or the company now owning the codebase? Regardless of if the reader thinks this matters or not, the context is now more dynamic. Technical documentation relates to the subject, independently of people and organizations. The author injecting themselves becomes a distraction.

For example, if we consider an alternative reality where we're documenting a proprietary hosted HTTP API, documenting rate-limiting behavior - this is one example of when I think a "we" could be warranted, depending on style.

Does that make sense?

Yeah! I hadn't thought about that but it makes perfect sense.

I'm not sure I got it right here but somehow I think that part can be simplified a bit?

Or something like that?

I actually had to do a reread to get this one. Let's see if we can make it more straightforward. I also think it can work better within a context (maybe it can be a section under common_tasks.md rather than an independent file?)

(Initial attempt, I'm not proposing it):

...Then I started writing about hosting a private registry using Verdaccio as an alternative.

Do you think it might make more sense to mention preview builds as one option among several (in which the instructions below can still be its own subsection)? I'd be happy to take a stab at part of it if it sounds good to you @mcmire - I think it shouldn't be too hard to get an unpermissioned option going as well.

And to you point in the PR comment, the contents of this file is what I would say might belong better in a blog post or article.

I found this a bit challenging to read as well.

Just to clarify though, the solution we're describing here is using a private registry. We're using the GitHub Packages registry. This "preview build" system is just a convenient way of publishing arbitrary commits.

Rather than manually publishing, a user can instead type @metamaskbot preview-build, and it will build and publish everything for you.

Regarding alternatives, publishing is the only strategy that we've found to work with CI. For local development there are alternatives, though it's not easy to explain all of the caveats to getting those working either.

This article might be improved by summarizing the problem scenario more concisely, then proceeding directly to the solution rather than enumerating alternatives first. We can leave alternative strategies for the end.

e.g.

I do appreciate the reasoning of your last proposal @Gudahtt . Just two things:

1. Can this be communicated without using "we"/"us"?
2. "We have setup scripts" could be read as referring to yarn setup, resulting in confusion. Making one of the words into a link to the relevant script would make things a lot more clear.

Take 2:

I'll work on rewriting this whole document to be less blog post-y / announcement-y and use the feedback here.

Just for my education: Why was this facilitated by breaking apart the packages? What made that unviable for the monopackage?

I'm not sure about this sentence. Previously, controller forks weren't (to my knowledge) used for experiments that often. They were used by mobile as a short/medium-term way to introduce controller changes quickly and release them in mobile, bypassing the need to make that change functional for the extension and agreeable for other teams. Though it has been over a year since I've seen that done (more recently they've used patches instead).

Experiments were done by installing deps from git references. Which was made impractical due to the Yarn v3 workspaces: protocol.

What we could list here as a benefit is that we've made it easier to fork individual controllers without everything else, reducing the maintenance burden of those forks.

Yeah, this sentence doesn't make any sense as a motivation, does it 😕 Admittedly, I editorialized the monorepo kickoff document a bit. Given that we aren't able to link to that document anyway I think I will just steal what you wrote @Gudahtt. Maybe the part about forking still makes sense but I'll work it in more as a benefit than a motivation as you said.

Authwalled link - can this be published?

Hmm. I guess we could publish it. It seems a bit in-the-weeds though, not especially relevant to external contributors, especially over time. And it has links to other internal docs.

We should at least note that it's an internal doc though (anyone with a ConsenSys account should have comment access).

I think I might remove this link entirely and work in more of the text itself into this document (assuming that it still makes sense to explain the motivations behind the monorepo). If our intent is to keep this private I don't want to risk it leaking out.

Hmm, this paragraph seems a bit... meandering. I like the idea of establishing what application state is; just saying "state" can be a bit too abstract to grasp. Maybe we can reformat this to separate the examples of application state from the details about some state being transient, some persisted, etc.

Fair!

Intended together with #989 (comment)

Or, in graph form:[^1]

![Dependency graph](assets/dependency-graph.png)

Refer to individual packages for usage instructions.



[^1]: To regenerate this graph, run `yarn generate-dependency-graph`.

Ah. I keep forgetting GitHub supports footnotes. That will put the note all the way at the bottom of the document. I guess that's okay — it's not something that needs to be referenced that often.

Not sure about the use of "location" here. The first time it seems like it means a location on disk, and the second it seems like you mean somewhere else in the codebase.

Maybe something like this would be clearer:

Ah, gotcha, good point.

I was trying to make it a bit more visualizable rather than abstract, and I feel like using the active form achieves that more than passive, but your rewording is more concise, so I like that. I'll play around with this a bit more.