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

#9430: tsconfig references refactor #9522

Merged
merged 14 commits into from
Nov 14, 2024
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ jobs:
node-version-file: applications/browser-extension/package.json
cache: npm
- run: npm ci
- run: npm run build:webpack --workspace=applications/browser-extension
- run: npm run build
env:
MV: 3
RELEASE_CHANNEL: stable
Expand Down Expand Up @@ -151,7 +151,7 @@ jobs:
node-version-file: applications/browser-extension/package.json
cache: npm
- run: npm ci
- run: npm run build:typecheck --workspaces
- run: npm run build:typecheck

lint:
runs-on: ubuntu-latest
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ jobs:
attempt_delay: 15000
attempt_limit: 40
- name: Build the extension
run: npm run build:webpack --workspace=applications/browser-extension
run: npm run build
- name: Run end to end tests
# Xvfb is required to run the tests in headed mode. Headed mode is required to run tests for browser extensions
# in Playwright, see https://playwright.dev/docs/ci#running-headed
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ browsers/dist
dist
.transpiled
web-ext-artifacts
*.tsbuildinfo

# Other artifacts
artifacts
Expand Down
10 changes: 10 additions & 0 deletions applications/browser-extension/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"baseUrl": ".",
"composite": false,
"declaration": false,
"noEmit": true,
"paths": {
"@/*": ["src/*"],
"@img/*": ["img/*"],
Expand All @@ -10,5 +13,12 @@
},
"plugins": [{ "name": "typescript-plugin-css-modules" }]
},
"references": [
// All project dependencies
// TODO: add @nx/js plugin which will automatically update these paths
{
"path": "../../libraries/utils"
}
],
"exclude": ["venv", "dist", "node_modules"]
}
4 changes: 4 additions & 0 deletions knip.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ const knipConfig = {
"prettier",
],
},
"libraries/*": {
entry: "src/*.ts!",
project: "**/*.ts",
},
Comment on lines +38 to +41
Copy link
Collaborator

@grahamlangford grahamlangford Nov 14, 2024

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?

Suggested change
"libraries/*": {
entry: "src/*.ts!",
project: "**/*.ts",
},
"libraries/*": {
// Files with a `!` suffix are included in production mode and default run
entry: "src/index.ts!",
project: ["src/**/*.ts!", "!src/**/*.d.ts!"],
},

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 or util-*: https://nx.dev/concepts/decisions/project-dependency-rules#utility-libraries

Copy link
Collaborator

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.

Copy link
Collaborator

@grahamlangford grahamlangford Nov 14, 2024

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't

Works with a more realistic test scenario (function exported from debugUtils but not index.ts instead)

Copy link
Collaborator Author

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.

"applications/browser-extension": {
entry: [
// ! suffix files are included in production mode
Expand Down
22 changes: 17 additions & 5 deletions libraries/utils/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,22 @@
module.exports = {
root: true,
extends: [
// Full config: https://github.com/pixiebrix/eslint-config-pixiebrix/blob/main/index.js
// XXX: use exact configuration for now because applications/browser-extension has a lot of local configuration.
// When we bring eslint-config-pixiebrix into the monorepo, we can iterate on code sharing across configs.
"pixiebrix",
overrides: [
{
files: ["**/*.ts", "**/*.tsx"],
extends: [
// Full config: https://github.com/pixiebrix/eslint-config-pixiebrix/blob/main/index.js
// XXX: use exact configuration for now because applications/browser-extension has a lot of local configuration.
// When we bring eslint-config-pixiebrix into the monorepo, we can iterate on code sharing across configs.
"pixiebrix",
],

parserOptions: {
// Use the nearest tsconfig.json in its directory
// See https://typescript-eslint.io/blog/parser-options-project-true/#introducing-true
fungairino marked this conversation as resolved.
Show resolved Hide resolved
project: ["./tsconfig.*?.json"],
tsconfigRootDir: __dirname,
},
},
],
};

Expand Down
4 changes: 2 additions & 2 deletions libraries/utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
"test": "TZ=UTC jest",
"lint": "eslint src --ext js,jsx,ts,tsx --quiet --report-unused-disable-directives",
"lint:fast": "ESLINT_NO_IMPORTS=1 eslint src --ext js,jsx,ts,tsx --quiet",
"build": "echo 'buildless package'",
"build:typecheck": "tsc --noEmit"
"build": "tsc --build",
"build:typecheck": "tsc --build"
},
"license": "AGPL-3.0",
"repository": "https://github.com/pixiebrix/pixiebrix-extension",
Expand Down
2 changes: 1 addition & 1 deletion libraries/utils/src/debugUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { getInvalidPath } from "@/debugUtils";
import { getInvalidPath } from "./debugUtils";
Copy link
Contributor

@twschiller twschiller Nov 13, 2024

Choose a reason for hiding this comment

The 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 @nx/js to set that up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is imported in the browser-extension like so:
import { getInvalidPath } from "@pixiebrix/utils/src/debugUtils";

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:

  "compilerOptions": {
    "paths": {
      "@/*": ["src/*"]

I don't feel too strongly either way about this.

Copy link
Contributor

@twschiller twschiller Nov 14, 2024

Choose a reason for hiding this comment

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

I'm not a big fan of the src directory appearing in the import, but it wouldn't be deal breaker. @pixiebrix/utils/debugUtils would seem the cleanest to me (like you're importing a normal package)

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:

See comment on top-level npm run watch: #9522 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not a big fan of the src directory appearing in the import, but it wouldn't be deal breaker. @pixiebrix/utils/debugUtils would seem the cleanest to me (like you're importing a normal package)

This should be simple enough to update with an exported index file as a follow-up


describe("getInvalidPath", () => {
it("returns invalid path", () => {
Expand Down
14 changes: 9 additions & 5 deletions libraries/utils/tsconfig.json
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"
}
}
]
}
19 changes: 19 additions & 0 deletions libraries/utils/tsconfig.lib.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"
]
}
19 changes: 19 additions & 0 deletions libraries/utils/tsconfig.test.json
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"
}
]
}
4 changes: 3 additions & 1 deletion nx.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,18 @@
"targetDefaults": {
"build": {
"dependsOn": ["^build"],
"outputs": ["{projectRoot}/dist"],
"outputs": ["{workspaceRoot}/dist"],
"cache": true
},
"test": {
"cache": true
},
"lint": {
"dependsOn": ["^build:typecheck"],
"cache": true
},
"build:typecheck": {
"dependsOn": ["^build:typecheck"],
"cache": true
}
},
Expand Down
4 changes: 3 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 9 additions & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@
"libraries/*"
],
"scripts": {
"test": "nx test",
"lint": "nx lint",
"test": "nx run-many -t test",
"lint": "nx run-many -t lint",
"build": "nx build",
"build:typecheck": "nx build:typecheck",
"build:typecheck": "nx run-many -t build:typecheck",
"dead-code": "npm run dead-code:base -- --include files,duplicates,dependencies,classMembers,binaries,enumMembers,nsTypes,exports,nsExports",
"dead-code:base": "knip --config knip.mjs --tags=-knip",
"dead-code:prod": "npm run dead-code -- --production",
"watch": "npm run watch --workspaces"
"watch": "concurrently npm:watch:webpack npm:watch:typescript -r",
"watch:typescript": "tsc --build --watch --preserveWatchOutput",
"watch:webpack": "npm run watch:webpack --workspaces"
},
"author": "Todd Schiller",
"license": "AGPL-3.0",
Expand All @@ -26,8 +28,10 @@
},
"devDependencies": {
"@sindresorhus/tsconfig": "^6.0.0",
"concurrently": "^9.1.0",
"knip": "^5.36.7",
"nx": "20.1.0",
"prettier": "3.3.3"
"prettier": "3.3.3",
"typescript": "^5.6.3"
}
}
6 changes: 3 additions & 3 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
"resolveJsonModule": true,
"baseUrl": ".",
"outDir": null,
"declaration": false,
"declaration": true,
"composite": true,

// TODO: Drop these lines to make TS stricter https://github.com/pixiebrix/pixiebrix-extension/issues/775
"strictFunctionTypes": false,
"noPropertyAccessFromIndexSignature": false,
"noImplicitReturns": false,
"noUnusedParameters": false
},
"exclude": ["node_modules"]
}
}
15 changes: 15 additions & 0 deletions tsconfig.json
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"
}
]
}
Loading