nrwl / nx

Smart Monorepos Β· Fast CI
https://nx.dev
MIT License
23.51k stars 2.34k forks source link

Make dep-graph logic extensible #2960

Closed merlosy closed 2 years ago

merlosy commented 4 years ago

Current Behavior

The dep-graph logic is not extensible by plugins.

The dep-graph logic was refactored to be more extensible but has not been made it public yet.

Expected Behavior

The dep-graph logic is extensible by plugins by providing hooks that would analyze files for their dependencies.

Notes

The dep-graph is extensible enough to handle different languages such as .NET and Java. If somebody from the community is interested in maintaining these kinds of plugins for Nx, please join us in the Community Slack to discuss what would be expected.

Original Description

Please make sure you have read the submission guidelines before posting an issue

Prerequisites

Please answer the following questions for yourself before submitting an issue. YOU MAY DELETE THE PREREQUISITES SECTION.

Expected Behavior

I managed to integrate some Vue applications inside a newly created NX monorepo. I have 2 Vue apps depending on a lib: they both import a Vue component (layout.vue) from it.

Is there a way to extend the TypeScriptImportLocator class to change the file extensions, or pass in some argument?

Current Behavior

When I run nx dep-graph, the link between the lib and the projects doesn't appear, because it doesn't parse .vue files.

image

Failure Information (for bugs)

By just adding extension !== '.vue' && into the "if" clause in this file, it works like a charm! But it's really tempering with the node modules, and i'd rather not.

Generated graph with the hack: image

Steps to Reproduce

I cannot easily share a repo with the issue, since it's an internal project in my company.

Context

Please provide any relevant information about your setup:

  @nrwl/angular : Not Found
  @nrwl/cli : 9.2.2
  @nrwl/cypress : 9.2.2
  @nrwl/eslint-plugin-nx : 9.2.2
  @nrwl/express : Not Found
  @nrwl/jest : 9.2.2
  @nrwl/linter : 9.2.2
  @nrwl/nest : Not Found
  @nrwl/next : Not Found
  @nrwl/node : Not Found
  @nrwl/react : Not Found
  @nrwl/schematics : Not Found
  @nrwl/tao : 9.2.2
  @nrwl/web : 9.2.2
  @nrwl/workspace : 9.2.2
  typescript : 3.8.3

I'm using latest Vue 2.x, with some plugins (even though it's not related):

Failure Logs

It doesn't fail explicitly. It just ignores vue files.

merlosy commented 4 years ago

Meanwhile, I've come up with a hack, until a better solution is found. It is far from ideal but I use a postinstall script:

#!/usr/bin node
const fs = require('fs');
const path = require('path');

/**
 * Patch dep-graph builder function to support Vue files.
 * @see https://github.com/nrwl/nx/issues/2960
 */
function patchNxDepGraph() {
  const file = 'node_modules/@nrwl/workspace/src/core/project-graph/build-dependencies/typescript-import-locator.js';
  try {
    const data = String(fs.readFileSync(path.resolve(file)));
    const replacement = 'extension !== \'.ts\' && extension !== \'.vue\' &&';
    if (data.indexOf(replacement) !== -1) {
      console.log('NX dep-graph for vue file support ALREADY patched');
      return;
    }
    const patched = data.replace('extension !== \'.ts\' &&', 'extension !== \'.ts\' && extension !== \'.vue\' &&');
    fs.writeFileSync(file, patched);
    console.log('NX dep-graph for vue file support patched');
  } catch (err) {
    console.error('Failed to patch dep-graph for vue file support', err);
  }
}

patchNxDepGraph();
FrozenPandaz commented 4 years ago

Wow, thanks for looking into this.

This is actually part of the reason why Vue support isn't trivial.

What would you expect for having a way to extend the way Nx finds dependencies between projects?

merlosy commented 4 years ago

Definitely not trivial. I'm not sure what's the best way to find dependencies between projects. I can propose different approaches:

I would opt in for the last option, as it seems more maintainable, but it requires the biggest amount of work. There may be other options to consider too, i'm open to suggestions of course... I'm still new with the codebase

merlosy commented 4 years ago

Any news about this? What is the recommended approach to fix this?

FrozenPandaz commented 4 years ago

Thank you for providing this insight. I edited the issue to making the dep-graph logic extensible in general.

I think the last method you mentioned is close to what I have in mind as well. We will be taking a look into this in the future :wink: but want to be careful when designing a solution here.

DominikPieper commented 4 years ago

@FrozenPandaz any more news on this or is this still the actual state? Sorry for jumping in

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs. If we missed this issue please reply to keep it active. Thanks for being a part of the Nx community! πŸ™

billyjov commented 3 years ago

Keep this active!

kucharskimaciej commented 3 years ago

Bump!

cmcnicholas commented 3 years ago

Bumpity bump

mblackritter commented 3 years ago

GRUMPETY GRUMBF! πŸ˜ΉπŸ’¦

That hint-line "Help us! We dislike this hack just as much as you do. Please give this Nx issue a πŸ‘ so that we can remove this hack in the future." could still not be removed... 😿

demisx commented 3 years ago

Any update on this, please? πŸ™πŸ»

merlosy commented 3 years ago

@FrozenPandaz Did you have enough time to think about the design for the fix?

dvnkboi commented 2 years ago

Sorry to jump in but any news on this?

AgentEnder commented 2 years ago

Can this be closed now with processProjectGraph available, or is there something more that is wanted? With that functionality, nx-dotnet as an example already extends the dep-graph edges.

jenjen75 commented 2 years ago

now from 13.10.3 the file path has changed to :

Meanwhile, I've come up with a hack, until a better solution is found. It is far from ideal but I use a postinstall script:

#!/usr/bin node
const fs = require('fs');
const path = require('path');

/**
 * Patch dep-graph builder function to support Vue files.
 * @see https://github.com/nrwl/nx/issues/2960
 */
function patchNxDepGraph() {
  const file = 'node_modules/nx/src/project-graph/build-dependencies/typescript-import-locator.js';
  try {
    const data = String(fs.readFileSync(path.resolve(file)));
    const replacement = 'extension !== \'.ts\' && extension !== \'.vue\' &&';
    if (data.indexOf(replacement) !== -1) {
      console.log('NX dep-graph for vue file support ALREADY patched');
      return;
    }
    const patched = data.replace('extension !== \'.ts\' &&', 'extension !== \'.ts\' && extension !== \'.vue\' &&');
    fs.writeFileSync(file, patched);
    console.log('NX dep-graph for vue file support patched');
  } catch (err) {
    console.error('Failed to patch dep-graph for vue file support', err);
  }
}

patchNxDepGraph();
AgentEnder commented 2 years ago

This feature landed a while back, in Nx 12.X. With support for the dep-graph extension documented, implemented, and released I'm going to close out this issue.

Here's a link to the docs: https://nx.dev/extending-nx/project-graph-plugins

If there is part of the extension api that needs further changes, feel free to open a new feature request detailing what is missing in the current implementation.

github-actions[bot] commented 1 year ago

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.