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
fix(js): mimic the behavior of tsc compilation for runTypeCheck #9240
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nrwl/nx-dev/GNmX4QVGiGqs2Wy1REJJt835PFg5 [Deployment for 41d8b18 canceled] |
520af70
to
7336272
Compare
7336272
to
1c4a90f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into this issue myself in an Ubuntu environment. I pulled and tested with this branch locally, and my builds are no longer failing (they were unable to resolve imports due to types being outputted to the wrong place).
This change looks solid to me, I'm not sure there is ever a case where outputPath.replace(`/${projectRoot}`, '')
made any sense. Users could simply change the value of the outputPath
if they didn't want the projectRoot
included in it.
Actually I did some more thinking on this, and I could see one alternative here. Imagine I have these settings in the project.json: {
projectRoot: "libs/mylibrary",
sourceRoot: "libs/mylibrary/src",
outputPath: "dist/libs/mylibrary"
} With this PR as it is, running
Perhaps a more logical layout might be
which could be accomplished with Just some food for thought |
@thecodeboss thank you for testing it. I'm not seeing the behavior you're seeing though {
"root": "packages/buildable-lib",
"sourceRoot": "packages/buildable-lib/src",
"targets": {
"build": {
"executor": "@nrwl/js:swc",
"outputs": ["{options.outputPath}"],
"options": {
"outputPath": "dist/packages/buildable-lib",
"main": "packages/buildable-lib/src/index.ts",
"tsConfig": "packages/buildable-lib/tsconfig.lib.json",
"assets": ["packages/buildable-lib/*.md"]
}
},
}
} So with this {
projectRoot: "packages/buildable-lib",
outputPath: "dist/packages/buildable-lib",
sourceRoot: "packages/buildable-lib/src"
} and running
Can you double check the configuration for your case? |
1c4a90f
to
44beb88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you also add a check for type defs location to existing e2e tests?
@nartc I was using an older version of nx@13.4.6 where I patched your changes onto it, so I just tested again on nx@13.9.3 and I get the expected behavior now. Sorry for the false alarm, thanks for checking! |
44beb88
to
41d8b18
Compare
* fix(js): mimic the behavior of tsc compilation for runTypeCheck ISSUES CLOSED: #9203 * chore(js): add check for type defs files in e2e Co-authored-by: Chau Tran <ctran@Chaus-MacBook-Pro.local>
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. |
ISSUES CLOSED: #9203
Current Behavior
On Windows, the
runTypeCheck
withemitDeclarationOnly
emits the type definitions to the wrong output directory.Expected Behavior
runTypeCheck
now mimics the behavior ofcompilationTypescript
(fromnrwl/workspace
whichnrwl/js:tsc
is utilizing) to ensure the behavior is consistent across different executors on different OS'sRelated Issue(s)
Fixes #9203