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

[RFC] New dependencies to Emission/Reaction go through the RFC process #117

Closed
orta opened this Issue Nov 27, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@orta
Copy link
Member

orta commented Nov 27, 2018

Proposal:

Fail PRs which add new keys to the dependencies section on the package.json in Reaction / Emission.

The fail would then be removed when an RFC is referenced in the body of the PR. Perhaps it canlook for a string like RFC: [ur] inside the body. Also support a message like #skip_rfc tag to skip the fail.

Reasoning

We'd like to find a better space to discuss additions to our two larger component libraries, it can be a shame to write a PR only to have it blocked on the discussion around adding a dependency

This is mainly to:

  • Keep our dependency tree somewhat under control
  • Ensure that we re-use dependencies, or avoid them if we have existing functionality you didn't know about
  • Give everyone the chance to see and react to dependency updates

Some recent examples:

Slack:

What an example RFC could look like

Exceptions:

Some PRs are pretty trivial, and so it should be possible to skip the discussion.

Additional Context:

You can see our discussion in notion (it's a bit light on notes because I was the one taking notes and talking at the same time )

@peril-staging peril-staging bot added the RFC label Nov 27, 2018

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Nov 27, 2018

An extension to this, we could opt to have a new template for these RFCs, which could look like:

### New Dependency

Name: danger
URL: https://danger.systems

### Focus

Allows us to start writing checks for things like changelog requirements, or requiring tests to simplify code review

### Check List

- [x] Have read over the source code, and we can maintain it
- [x] Has had a release in the last year, or looks done and stable
- [ ] ...

### Alternatives

- Setting up Peril

  This is a bit too much to just jump to right now

- Rubocop

  Only works for ruby projects, and we have iOS / JS projects too

(The check list could come from @alloy's work on dependency rubrics)

@ashfurrow

This comment has been minimized.

Copy link
Member

ashfurrow commented Nov 27, 2018

I like this. I'd rather swing too much in favour of process and then pull back, than to continue without enough.

@damassi

This comment has been minimized.

Copy link
Member

damassi commented Nov 27, 2018

I'm into it too -- let us use Force's package.json as an example! (And our totally massive bundle sizes.)

@orta - Does this also apply to dev tooling / devDependencies? (I imagine this as only relating to deployable dist code.)

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Nov 27, 2018

Not to devDependencies, no - only distributed code

@javamonn

This comment has been minimized.

Copy link
Contributor

javamonn commented Nov 28, 2018

We should do what we can to encourage that the necessary discussion happens upfront - failing a PR is probably too late as usually by that time your implementation is pretty coupled to the dependency that you introduced.

This will introduce a some overhead in some product work ("why is this small ticket taking so long?") but perfectly fits the description of a "spike" ticket.

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Dec 3, 2018

We've shipped a doc that gives heuristics on whether a dev should have an RFC In https://github.com/artsy/README/blob/master/playbooks/dependencies.md

Will add a Peril rule that detects the changes and notes that it may need an RFC

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Feb 6, 2019

OK, I'm going to close this as "we agree on it"

and I'll ship an RFC template ASAP

@orta orta closed this Feb 6, 2019

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.