fix: moved CommonCommands to separate file #16522
Conversation
eneufeld
force-pushed
the
fix/circular-dependency-common-commands
branch
2 times, most recently
from
58ec27d to
8b293f9
Compare
github-project-automation
bot
moved this from Waiting on reviewers
to Needs merge
in PR Backlog
|
@ndoschek I think it would be good to add circular dependency analysis to the (CI) builds in some way so we can avoid issues like this in the future. They always hit at inconvenient times as they often "work" but are brittle to changes and can be triggered with slightly different usage patterns |
|
Thank you @sdirix for raising this, that sounds good. I'll create a ticket and plan it in for this month to have this check implemented soon. |
With CommandComamnds being part of the packages/core/src/browser/common-frontend-contribution.ts file we get circular dependencies. This leads to issues especially when running tests. The Commands are now moved to an own file. This is a breaking change for whoever does a deep import of the CommonCommands namespace.
eneufeld
force-pushed
the
fix/circular-dependency-common-commands
branch
from
8b293f9 to
d89ce28
Compare
| import { FrontendApplicationContribution, OnWillStopAction } from './frontend-application-contribution'; | ||
| import { CommandContribution, CommandRegistry, Command } from '../common/command'; | ||
| import { CommonCommands } from './common-commands'; | ||
| export { CommonCommands }; |
There was a problem hiding this comment.
With this change, the breaking change notice can probably be removed from the changelog: external consumers shouldn't be affected.
|
|
||
| } | ||
|
|
||
| export namespace CommonCommands { |
There was a problem hiding this comment.
The CommonMenus namespace immediately above this is similar to CommonCommands in that it consists entirely of dependency-free constants and is imported in a few other places, raising the risk of circular imports. We could move it out to its own file, as well.
(It's also involved in at least one circular dependency)
What it does
With CommandComamnds being part of the packages/core/src/browser/common-frontend-contribution.ts file we get circular dependencies. This leads to issues especially when running tests.
The Commands are now moved to an own file. This is a breaking change for whoever does a deep import of the CommonCommands namespace.
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers