Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Conversation

@chrismwendt
Copy link
Contributor

cc @sourcegraph/frontend-devs

@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #408 (c49dd20) into master (a7aa682) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #408   +/-   ##
=======================================
  Coverage   85.91%   85.91%           
=======================================
  Files          14       14           
  Lines         653      653           
  Branches      175      175           
=======================================
  Hits          561      561           
  Misses         92       92           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7aa682...c49dd20. Read the comment docs.

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

If this version match is important, should we make it a peer dependency instead?

@chrismwendt
Copy link
Contributor Author

It sounds like peer dependencies aren't very ergonomic to work with: https://dev.to/yvonnickfrin/how-to-handle-peer-dependencies-when-developing-modules-18fa

Do you have enough knowledge about peer dependencies to recommend them? I'm vaguely familiar with the concept, but I don't know all the quirks.

@eseliger
Copy link
Member

I haven't used them much in dev, only found them handy when some package I depend on requires React for example, that it'll always use the version I provide and not bundle a second one.

@chrismwendt
Copy link
Contributor Author

I'm hesitant to use peer deps because I'm not sure how we can make sure it does what we expect in different scenarios:

  • Running tests (needs the dep)
  • yarn link in sourcegraph/sourcegraph (should not have the dep)

I can imagine juggling that being confusing/annoying. Keeping the version in sync with sourcegraph/sourcegraph seems more straightforward in my mind right now, but I'm not opposed to trying peer deps sometime.

@chrismwendt chrismwendt merged commit 457551c into master Oct 27, 2021
@chrismwendt chrismwendt deleted the rxjs-6-6-3 branch October 27, 2021 21:21
@eseliger
Copy link
Member

I think a devDependency could fix the testing part. Not worth breaking a sweat rn though.

@chrismwendt
Copy link
Contributor Author

I wonder what would happen if you used a devDependency and then yarn link into sourcegraph/sourcegraph. I'm guessing rxjs would be present when it shouldn't, but I don't know for sure.

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.

3 participants