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

esbuild - paths from tsconfig.base.json do not work properly in the build output #20817

Closed
2 of 4 tasks
th1nkful opened this issue Dec 18, 2023 · 8 comments · Fixed by #27092
Closed
2 of 4 tasks

esbuild - paths from tsconfig.base.json do not work properly in the build output #20817

th1nkful opened this issue Dec 18, 2023 · 8 comments · Fixed by #27092
Assignees
Labels
outdated scope: bundlers Issues related to webpack, rollup type: bug

Comments

@th1nkful
Copy link

Current Behavior

When using imports that have similar paths, esbuild will not output a manifest in main.js that properly allows importing all libs if they are ordered in certain orders in the tsconfig.base.json

For example:

The following paths in tsconfig.base.json will not order correctly in the manifest generated for here:

const manifest = ${JSON.stringify(manifest)};

"path": {
      "@worklabs/db": ["libs/db/core/src/index.ts"],
      "@worklabs/db/migrations": ["libs/db/migrations/src/index.ts"],
      "@worklabs/db/models": ["libs/db/models/src/index.ts"],
}

Because @worklabs/db was first in paths, any imports when going through the hijacked _resolveFilename that were @worklabs/db/migrations or @worklabs/db/models were directed to @worklabs/db files which did not work

Interestingly, ts in VSCode found no issues with the order of paths. Nor did serve using @nx/js:node

Expected Behavior

If I have the above entries, they should be sorted so that the longer ones are first and do not mistakenly get matched to the wrong import

Locally, I was able to fix this behaviour by editing getRegisterFileContent function to sort by whole string length first like so

const sortedKeys = Object.keys(paths)
      .sort((a, b) => b.length - a.length)
      .sort((a, b) => getPrefixLength(b) - getPrefixLength(a));

GitHub Repo

No response

Steps to Reproduce

  1. Add libs that use subpaths, adding the longer ones secondary
  2. Try build with esbuild
  3. Try use the build with the outputs

Nx Report

Convert compiler options from json failed, No inputs were found in config file 'tsconfig.json'. Specified 'include' paths were '["node_modules/jest-extended/types/index.d.ts"]' and 'exclude' paths were '["node_modules","dist","build","coverage"]'.

 >  NX   Report complete - copy this into the issue template

   Node   : 20.10.0
   OS     : darwin-arm64
   pnpm   : 8.11.0
   
   nx (global)        : 17.2.0
   nx                 : v17.2.5
   @nx/js             : v17.2.5
   @nx/jest           : 17.2.0
   @nx/linter         : v17.2.5
   @nx/eslint         : v17.2.5
   @nx/workspace      : v17.2.5
   @nx/cypress        : v17.2.5
   @nx/devkit         : v17.2.5
   @nx/esbuild        : 17.2.0
   @nx/eslint-plugin  : v17.2.5
   @nx/node           : v17.2.5
   @nx/react          : v17.2.5
   @nx/storybook      : v17.2.5
   @nrwl/tao          : v17.2.5
   @nx/vite           : v17.2.5
   @nx/web            : v17.2.5
   @nx/webpack        : 17.2.0
   typescript         : 5.3.3
   ---------------------------------------
   The following packages should match the installed version of nx
     - @nx/[email protected]
     - @nrwl/[email protected]
     - @nx/[email protected]
     - @nrwl/[email protected]
     - @nx/[email protected]
     - @nrwl/[email protected]
   
   To fix this, run `nx migrate [email protected]`

Failure Logs

Depending on the module we get different errors:

If the output is valid, but the contents are different we get something like: 

migrations.runMigrations is not a function

Otherwise we get errors about reading a property of undefined

Package Manager Version

8.11.0

Operating System

  • macOS
  • Linux
  • Windows
  • Other (Please specify)

Additional Information

We picked this up while migrating to NX for "almost-monorepo" (code was all in one repo, but structure and tooling was lagging), so this wasn't something that was working before and now doesn't

@th1nkful
Copy link
Author

Let me know if there is something more I can do to help here out

My short term fix has been to sort my "paths" config in tsconfig.base.json so that it has the longer in those situations at the top. This seems to work but isn't a great long term experience as Nx inserts into paths where it wants to so will be something we have to keep an eye on as new libs are added

@AgentEnder AgentEnder added the scope: bundlers Issues related to webpack, rollup label Dec 21, 2023
@jelling
Copy link

jelling commented Feb 16, 2024

Interestingly, ts in VSCode found no issues with the order of paths. Nor did serve using @nx/js:node

In my case, the issue was also happening with serve using @nx/js:node. I'm manually fixing the sort for the moment.

@th1nkful
Copy link
Author

@jelling I would recommend adding a quick script you can run to sort the paths by length (that’s what we did)

every time you make Nx change (add a lib for example) it reorders the paths and you have to manually fix it up. Sorting by length with a script makes it trivial to fix the order - just my 2c

@jelling
Copy link

jelling commented Feb 19, 2024

Are you running this script manually or as part of the build process? If the latter, could you please show an example? Thanks.

@th1nkful
Copy link
Author

@jelling atm we are just running it manually as we don’t add a lot of libs

We aren’t doing this but you could add a target using nx run-command and make that a dependency of your build job. Could do something like what was discussed here: #9054

@jelling
Copy link

jelling commented Feb 21, 2024

@th1nkful could you please share the script?

@th1nkful
Copy link
Author

@jelling sure! We just use ts-node to run this from scripts in our package.json

"tspaths:sort": "ts-node ./tools/nx-tsconfig-paths-fix/index.ts"
import fs from 'fs';
import path from 'path';

const tsconfigPath = path.resolve(__dirname, '../../tsconfig.base.json');
const file = fs.readFileSync(tsconfigPath, 'utf8');
const config = JSON.parse(file);

const paths = config.compilerOptions.paths;
const newPaths: Record<string, any> = {};

const keys = Object.keys(paths);
keys.sort((a, b) => {
  const lengthCompare = b.length - a.length;
  if (lengthCompare !== 0) return lengthCompare;
  return a.localeCompare(b);
});

for (const key of keys) {
  newPaths[key] = paths[key];
}

config.compilerOptions.paths = newPaths;

fs.writeFileSync(tsconfigPath, JSON.stringify(config, null, 2));

@FrozenPandaz FrozenPandaz assigned Coly010 and unassigned ndcunningham Jul 22, 2024
ndcunningham pushed a commit that referenced this issue Jul 24, 2024
…27092)

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->
When there are workspace libraries with the following import paths:

```
@org/core
@org/core/db
@org/core/acme
```

They need to be sorted for the manifest such that matching finds the
most specific first.
The logic for this is currently based off whether an `*` exists, which
doesn't work when the paths are specific.


## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->
Try to use `*` first and fallback to `/` to determine package prefix
length.


## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #20817
FrozenPandaz pushed a commit that referenced this issue Jul 24, 2024
…27092)

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->
When there are workspace libraries with the following import paths:

```
@org/core
@org/core/db
@org/core/acme
```

They need to be sorted for the manifest such that matching finds the
most specific first.
The logic for this is currently based off whether an `*` exists, which
doesn't work when the paths are specific.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->
Try to use `*` first and fallback to `/` to determine package prefix
length.

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #20817

(cherry picked from commit a8dc251)
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated scope: bundlers Issues related to webpack, rollup type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants