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

feat(core): show dep types in dep graph (#2760) #8132

Merged
merged 1 commit into from Jan 18, 2022

Conversation

MaximSagan
Copy link
Contributor

@MaximSagan MaximSagan commented Dec 13, 2021

Current Behavior

Dep graph does not indicate type-only dependencies or dynamic (a.k.a. "lazy") dependencies.
(Note: Type-only dependencies have never been indicated; dynamic dependencies are currently not indicated due to a relatively-recent regression.)

Expected Behavior

Dep graph indicates type-only, static, dynamic and implicit dependencies.

Fixes #2760

@vercel
Copy link

vercel bot commented Dec 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/FMVWx2RCHeGECS3KtbyoNuyfep3w
✅ Preview: https://nx-dev-git-fork-maximsagan-feature-dependency-type-nrwl.vercel.app

@MaximSagan
Copy link
Contributor Author

MaximSagan commented Dec 13, 2021

This PR is a little bigger than expected, because I had to fix a regression I found. Specifically, since this commit, the dependency types identified using TypescriptImportLocator are no longer being passed to the ProjectGraphBuilder. (This also meant that any discovered dynamic imports were not being identified on the graph.)

@nx-cloud
Copy link

nx-cloud bot commented Dec 13, 2021

☁️ Nx Cloud Report

CI ran the following commands for commit 0c8d563. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 21 targets

Sent with 💌 from NxCloud.

@@ -245,7 +268,7 @@ describe('project graph', () => {
target: 'shared-util',
},
{
type: DependencyType.static,
type: DependencyType.dynamic,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to value before 0e52c43

@@ -201,7 +224,7 @@ describe('project graph', () => {
{
source: 'ui',
target: 'lazy-lib',
type: 'static',
type: 'dynamic',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to value before 0e52c43

{
source: 'demo',
target: 'lazy-lib',
type: 'static',
type: DependencyType.dynamic,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to value before 0e52c43

@@ -33,7 +33,7 @@ export class MockProjectGraphService implements ProjectGraphService {
{
source: 'existing-app-1',
target: 'existing-lib-1',
type: 'statis',
type: 'static' as any,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I narrowed the type of the property ProjectGraphDependency['type'] from DependencyType | string to simply DependencyType as it is now being used a little bit more and for the most part the narrower type is helpful. However, for some reason, in this module, importing the enum DependencyType from @nrwl/devkit was causing an extremely bizarre error in the dep-graph-dep-graph:build-base task, which I captured here:

ERROR in ../../packages/devkit/index.ts
Module build failed (from ../../node_modules/@nrwl/web/src/utils/web-babel-loader.js):
SyntaxError: C:\dev\nx\packages\devkit\index.ts: Support for the experimental syntax 'flow' isn't currently enabled (15:8):

  13 |  * @category Tree
  14 |  */
> 15 | export type { Tree, FileChange } from '@nrwl/tao/src/shared/tree';
     |        ^
  16 |
  17 | /**
  18 |  * @category Workspace

Add @babel/preset-flow (https://git.io/JfeDn) to the 'presets' section of your Babel config to enable transformation.
If you want to leave it as-is, add @babel/plugin-syntax-flow (https://git.io/vb4yb) to the 'plugins' section to enable parsing.

What makes it even more bizarre is that there are a number of other modules in this same app that are importing the enum DependencyType without any issue. My only guess is that the difference here is the presence of the decorator, but why that would cause the above error is beyond me.

At any rate, adding as any seemed like a decent compromise.

'line-dash-pattern': [5, 5],
'line-style': 'dashed',
},
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philipjfulcher
I noticed you copied the contents of this file over to the React app in #8152. Note that if by some chance my PR is accepted and merged before yours, this edge definition should be copied too.
If yours PR is merged first, I'll add this to the React app.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My PR was merged. :) Let me know if you have trouble rebasing this, I can help with conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My PR was merged. :) Let me know if you have trouble rebasing this, I can help with conflicts.

Seems I was able to rebase it safely. Would you mind reviewing?

@MaximSagan
Copy link
Contributor Author

@vsavkin @FrozenPandaz
Can I ask someone from the team to take a look at this? The dependency graph is being actively worker on, so I've already had to resolve conflicts and rebase a couple times.

@vsavkin vsavkin force-pushed the master branch 2 times, most recently from 3a99403 to 334c230 Compare January 6, 2022 18:35
@philipjfulcher
Copy link
Collaborator

@MaximSagan - Thanks for your contribution! I've been out of the office since December and just getting back to this. It looks like we need another rebase, but it seems like an easy one. The changes to dep-graph-client look good, I need someone else's opinion on the other changes. If you can push a rebase, I'll get someone else to look at it.

@MaximSagan
Copy link
Contributor Author

@MaximSagan - Thanks for your contribution! I've been out of the office since December and just getting back to this. It looks like we need another rebase, but it seems like an easy one. The changes to dep-graph-client look good, I need someone else's opinion on the other changes. If you can push a rebase, I'll get someone else to look at it.

Thanks @philipjfulcher. Rebased.

@philipjfulcher philipjfulcher merged commit 31bb2f3 into nrwl:master Jan 18, 2022
vsavkin added a commit that referenced this pull request Jan 25, 2022
@spaceribs
Copy link
Contributor

@vsavkin can this be reopened?

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023
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.

Show library typing/runtime dependencies differently in dependency graph
3 participants