-
Notifications
You must be signed in to change notification settings - Fork 23
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
#9430: tsconfig references refactor #9522
Changes from 13 commits
ec7e59d
db6b2f3
9747e79
9925067
e2a700f
8112e56
79cfd1b
0c76026
0ff1119
8c91677
9a3a285
f76130b
149dfa5
f6bfacf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ browsers/dist | |
dist | ||
.transpiled | ||
web-ext-artifacts | ||
*.tsbuildinfo | ||
|
||
# Other artifacts | ||
artifacts | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -15,7 +15,7 @@ | |||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||||
*/ | ||||
|
||||
import { getInvalidPath } from "@/debugUtils"; | ||||
import { getInvalidPath } from "./debugUtils"; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the import convention from inside the browser-extension workspace? Or do we have to wait for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is imported in the browser-extension like so: I removed this following the suggested tsconfigs from nx, but I think we could add back the following in the libraries/utils tsconfig if we wanted:
I don't feel too strongly either way about this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a big fan of the I'm generally not a fan of relative imports (for reasons, e.g., listed on the lint rule https://www.npmjs.com/package/eslint-plugin-no-relative-import-paths), but with the same directory a potential exception Where in the slicing are we introducing an actual cross-workspace import to the build? Update: I see that the example import is here: pixiebrix-extension/applications/browser-extension/src/pageEditor/store/editor/editorSlice.ts Line 771 in 0c76026
See comment on top-level There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This should be simple enough to update with an exported index file as a follow-up |
||||
|
||||
describe("getInvalidPath", () => { | ||||
it("returns invalid path", () => { | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,13 @@ | ||
{ | ||
"extends": "../../tsconfig.base.json", | ||
"compilerOptions": { | ||
"baseUrl": ".", | ||
"paths": { | ||
"@/*": ["src/*"] | ||
"files": [], | ||
"include": [], | ||
"references": [ | ||
{ | ||
"path": "./tsconfig.lib.json" | ||
}, | ||
{ | ||
"path": "./tsconfig.test.json" | ||
} | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"extends": "../../tsconfig.base.json", | ||
"compilerOptions": { | ||
"baseUrl": ".", | ||
"rootDir": "src", | ||
"outDir": "dist", | ||
"tsBuildInfoFile": "dist/tsconfig.lib.tsbuildinfo", | ||
"emitDeclarationOnly": false, | ||
"types": ["node"] | ||
}, | ||
"include": ["src/**/*.ts", "src/**/*.d.ts"], | ||
"references": [], | ||
"exclude": [ | ||
"jest.config.ts", | ||
"jest.config.js", | ||
"src/**/*.spec.ts", | ||
"src/**/*.test.ts" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"extends": "../../tsconfig.base.json", | ||
"compilerOptions": { | ||
"outDir": "../../dist/out-tsc/libraries/utils", | ||
"types": ["jest", "node"] | ||
}, | ||
"include": [ | ||
"jest.config.ts", | ||
"jest.config.js", | ||
"src/**/*.test.ts", | ||
"src/**/*.spec.ts", | ||
"src/**/*.d.ts" | ||
], | ||
"references": [ | ||
{ | ||
"path": "./tsconfig.lib.json" | ||
} | ||
] | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
{ | ||
"extends": "./tsconfig.base.json", | ||
"compileOnSave": false, | ||
"files": [], // intentionally empty | ||
"references": [ | ||
// All projects in the repository | ||
// TODO: add @nx/js plugin which will automatically update these paths | ||
{ | ||
"path": "./applications/browser-extension" | ||
}, | ||
{ | ||
"path": "./libraries/utils" | ||
} | ||
] | ||
} |
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.
Should we really be making every file in the src folder an entry? What if a utility function exports a helper function for testing but shouldn't be used outside the library?
This way we force engineers to define in the index.ts exactly what functions are available outside the library
Second, NX recommends we call the library either
util
orutil-*
: https://nx.dev/concepts/decisions/project-dependency-rules#utility-librariesThere 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 tested the original code by creating a testFn.ts that exported a simple function and a testFn.test.ts that imported it. It's used nowhere else. The current config doesn't catch it when running production mode. The suggested config does.
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.
Wait, I checked whether the @internal disable would work, looks like it doesn'tWorks with a more realistic test scenario (function exported from debugUtils but not index.ts instead)
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 totally agree with this suggestion, I will make this change in a follow-up since this aligns with the same feedback that Todd left further up on avoiding importing from the src directory.