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

Support package.json repository.directory field #3096

Closed
felixfbecker opened this issue Jan 21, 2019 · 12 comments
Closed

Support package.json repository.directory field #3096

felixfbecker opened this issue Jan 21, 2019 · 12 comments
Labels
manager:npm package.json files (npm/yarn/pnpm) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)

Comments

@felixfbecker
Copy link

Not sure if you're already aware of this, but npm just ratified an RFC for specifying the directory in monorepos for packages: https://github.com/npm/rfcs/blob/latest/accepted/0010-monorepo-subdirectory-declaration.md
It already got adopted by packages like React

@rarkins rarkins added type:feature Feature (new functionality) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others manager:npm package.json files (npm/yarn/pnpm) labels Jan 22, 2019
@rarkins
Copy link
Collaborator

rarkins commented Jan 22, 2019

Thanks for the heads-up. We already have logic that checks within subdirectories for matching packages in a monorepo, which catches most of the cases without using this field. Adding explicit knowledge of this will give us a more accurate "source" link in PRs though, because the existing logic only covers the changelogs themselves.

@renovate-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 13.180.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SimenB
Copy link

SimenB commented Mar 8, 2019

Hmm, this doesn't seem to work? Jest added these fields here: jestjs/jest#7649

But we just got a PR where the source link in the OP still points to the root of the repo:
image

@rarkins rarkins reopened this Mar 8, 2019
@rarkins rarkins closed this as completed in d78e6a0 Mar 8, 2019
@rarkins
Copy link
Collaborator

rarkins commented Mar 8, 2019

Verified here: renovate-tests/sourcedir1#3

Should be live within the app shortly so updated in your repo within 1-2 hours max

@renovate-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 14.42.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SimenB
Copy link

SimenB commented Mar 8, 2019

Nice! The link is now updated, but it doesn't work - if you click the link, GitHub gives you 404

The link produced is https://github.com/facebook/jest/packages/jest-environment-node
It should be https://github.com/facebook/jest/tree/master/packages/jest-environment-node

Or maybe https://github.com/facebook/jest/tree/v24.3.1/packages/jest-environment-node to not be coupled to master

@rarkins rarkins reopened this Mar 8, 2019
@rarkins rarkins closed this as completed in 2376ae4 Mar 8, 2019
@rarkins
Copy link
Collaborator

rarkins commented Mar 8, 2019

I've gone for tree/master for now, but longer term a better solution is needed.

The reason for not linking to the tag is:

  1. We don't link to source repos by tag currently (when non-nested) and I'm undecided if it's the best approach, and if so then should it be done consistently for both nested and non-nested packages?
  2. We can't be sure that the tagged URL exists without doing some extra checking that I prefer to avoid. We'll probably do it with a long cache expiry to save unnecessarily hammering GitHub's API

Also to note here: we can't even be sure that master is the default branch anyway :-/

@SimenB
Copy link

SimenB commented Mar 8, 2019

We don't link to source repos by tag currently (when non-nested) and I'm undecided if it's the best approach, and if so then should it be done consistently for both nested and non-nested packages?

Yeah, I think so. You say "this dependency went from version x to version y, here's the source code for version y". The tag might be missing though, you're right... I don't have any good ideas for that.

Thanks for being super responsive, btw 🙂

@rarkins
Copy link
Collaborator

rarkins commented Mar 8, 2019

Right now there is a (subtle) difference between the "source URL" link vs viewing or diffing the source code.

For now, the source URL is meant to mean "Here is where the source code for this package is located" as distinct from "Here is the home page of this package", and not necessarily meant as a snapshot in time of that source code.

Meanwhile, we also try to link to the diffs in different ways - both via GitHub tags as well as now via Intrinsic's npm diffs. For example here is a jest-circus link: https://diff.intrinsic.com/jest-circus/24.3.0/24.3.1

Thanks for being super responsive, btw 🙂

You're welcome, and your feedback is always appreciated

@renovate-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 14.42.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@felixfbecker
Copy link
Author

You can replace master with HEAD to link to the tip of the default branch

@rarkins
Copy link
Collaborator

rarkins commented Mar 8, 2019

@felixfbecker great idea, thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
manager:npm package.json files (npm/yarn/pnpm) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)
Projects
None yet
Development

No branches or pull requests

4 participants