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

refactor(core)!: replace workspace.dependencies with worskpace.anchoredPackage #4898

Merged
merged 4 commits into from
Oct 7, 2022

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Sep 24, 2022

What's the problem this PR addresses?

I don't like workspace.dependencies because the Project class has to manually keep it in sync (and historically it hasn't done a good job).

How did you fix it?

I removed it and added a new workspace.anchoredPackage getter that can be used in more situations.

Since there is only one production codepath using workspace.dependencies and it accesses it a single time, the lack of caching for workspace.anchoredPackage doesn't affect its performance.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@paul-soporan paul-soporan mentioned this pull request Sep 24, 2022
13 tasks
@arcanis
Copy link
Member

arcanis commented Oct 5, 2022

API-wise, what about turning it into a getter named anchoredPackage? It'd nicely mirror the existing anchoredLocator 🤔

@paul-soporan paul-soporan changed the title refactor(core)!: replace workspace.dependencies with getPackage refactor(core)!: replace workspace.dependencies with worskpace.anchoredPackage Oct 5, 2022
@paul-soporan
Copy link
Member Author

I like this idea, even though I generally don't like getters.
Since in this case workspace.anchoredPackage is O(1) anyways, it doesn't really hide expensive computations, and we already have other such getters (project.topLevelWorkspace), so it's fine by me (especially since it nicely mirrors workspace.anchoredLocator as you said).

I've updated the PR 👍

@arcanis arcanis merged commit 578dbe7 into master Oct 7, 2022
@arcanis arcanis deleted the paul/refactor/remove-workspace.dependencies branch October 7, 2022 10:31
@merceyz merceyz added this to the 4.0.0 milestone May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants