From 5322a718e8bf6589b9bcb0b33889e0a024e2b04d Mon Sep 17 00:00:00 2001 From: Cristiano Belloni Date: Tue, 20 Dec 2022 14:14:10 +0000 Subject: [PATCH] modular `source` type (#2228) * Allow circular dependencies for source packages * Remove non-buildable workspaces from build anyway * Better package types utilities * Typos + fix test * Better start check and error message * Remove default export * Simplify Modular type checks * Split selectWorkspaces * Introduce --dangerouslyIgnoreCircularDependencies * Add circular dependencies docs * Add fixtures and tests * Add source type to add command + docs * update snapshots * Create short-laws-stare.md * Fix typo --- .changeset/short-laws-stare.md | 5 + __fixtures__/source-type/.editorconfig | 18 ++ __fixtures__/source-type/.eslintignore | 23 +++ __fixtures__/source-type/.gitignore | 24 +++ __fixtures__/source-type/.prettierignore | 2 + __fixtures__/source-type/.yarnrc | 1 + __fixtures__/source-type/README.md | 1 + .../source-type/modular/setupEnvironment.ts | 2 + .../source-type/modular/setupTests.ts | 5 + __fixtures__/source-type/package.json | 60 ++++++ .../source-type/packages/a/package.json | 12 ++ .../source-type/packages/a/src/index.ts | 3 + .../source-type/packages/b/package.json | 12 ++ .../source-type/packages/b/src/index.ts | 3 + .../source-type/packages/c/package.json | 13 ++ .../source-type/packages/c/src/index.ts | 3 + .../source-type/packages/d/package.json | 9 + .../source-type/packages/d/src/index.ts | 3 + .../source-type/packages/e/package.json | 12 ++ .../source-type/packages/e/src/index.ts | 3 + __fixtures__/source-type/tsconfig.json | 4 + docs/commands/add.md | 14 +- docs/concepts/circular-dependencies.md | 78 ++++++++ .../src/__tests__/selectiveBuild.test.ts | 2 +- .../src/__tests__/sourceType.test.ts | 68 +++++++ .../getWorkspaceInfo.test.ts.snap | 19 +- packages/modular-scripts/src/addPackage.ts | 9 + packages/modular-scripts/src/build/index.ts | 11 +- .../src/check/verifyWorkspaceDependencies.ts | 11 +- packages/modular-scripts/src/program.ts | 16 ++ packages/modular-scripts/src/serve.ts | 2 +- packages/modular-scripts/src/start.ts | 16 +- .../src/utils/isModularType.ts | 41 ---- .../modular-scripts/src/utils/packageTypes.ts | 60 ++++++ .../src/utils/selectWorkspaces.ts | 187 +++++++++++++----- packages/modular-template-source/package.json | 17 ++ .../src/__tests__/index.test.ts | 5 + packages/modular-template-source/src/index.ts | 3 + packages/modular-types/src/types.ts | 7 +- 39 files changed, 673 insertions(+), 111 deletions(-) create mode 100644 .changeset/short-laws-stare.md create mode 100644 __fixtures__/source-type/.editorconfig create mode 100644 __fixtures__/source-type/.eslintignore create mode 100644 __fixtures__/source-type/.gitignore create mode 100644 __fixtures__/source-type/.prettierignore create mode 100644 __fixtures__/source-type/.yarnrc create mode 100644 __fixtures__/source-type/README.md create mode 100644 __fixtures__/source-type/modular/setupEnvironment.ts create mode 100644 __fixtures__/source-type/modular/setupTests.ts create mode 100644 __fixtures__/source-type/package.json create mode 100644 __fixtures__/source-type/packages/a/package.json create mode 100644 __fixtures__/source-type/packages/a/src/index.ts create mode 100644 __fixtures__/source-type/packages/b/package.json create mode 100644 __fixtures__/source-type/packages/b/src/index.ts create mode 100644 __fixtures__/source-type/packages/c/package.json create mode 100644 __fixtures__/source-type/packages/c/src/index.ts create mode 100644 __fixtures__/source-type/packages/d/package.json create mode 100644 __fixtures__/source-type/packages/d/src/index.ts create mode 100644 __fixtures__/source-type/packages/e/package.json create mode 100644 __fixtures__/source-type/packages/e/src/index.ts create mode 100644 __fixtures__/source-type/tsconfig.json create mode 100644 docs/concepts/circular-dependencies.md create mode 100644 packages/modular-scripts/src/__tests__/sourceType.test.ts delete mode 100644 packages/modular-scripts/src/utils/isModularType.ts create mode 100644 packages/modular-scripts/src/utils/packageTypes.ts create mode 100644 packages/modular-template-source/package.json create mode 100644 packages/modular-template-source/src/__tests__/index.test.ts create mode 100644 packages/modular-template-source/src/index.ts diff --git a/.changeset/short-laws-stare.md b/.changeset/short-laws-stare.md new file mode 100644 index 000000000..9a161523f --- /dev/null +++ b/.changeset/short-laws-stare.md @@ -0,0 +1,5 @@ +--- +"modular-scripts": minor +--- + +modular `source` type + `--dangerouslyIgnoreCircularDependencies` build option diff --git a/__fixtures__/source-type/.editorconfig b/__fixtures__/source-type/.editorconfig new file mode 100644 index 000000000..7526bd7ab --- /dev/null +++ b/__fixtures__/source-type/.editorconfig @@ -0,0 +1,18 @@ +# https://editorconfig.org +root = true + +[*] +charset = utf-8 +end_of_line = lf +indent_size = 2 +indent_style = space +insert_final_newline = true +max_line_length = 80 +trim_trailing_whitespace = true + +[*.{md,mdx}] +max_line_length = 0 +trim_trailing_whitespace = false + +[COMMIT_EDITMSG] +max_line_length = 0 diff --git a/__fixtures__/source-type/.eslintignore b/__fixtures__/source-type/.eslintignore new file mode 100644 index 000000000..d529732cc --- /dev/null +++ b/__fixtures__/source-type/.eslintignore @@ -0,0 +1,23 @@ +# See https://help.github.com/articles/ignoring-files/ for more about ignoring files. + +# dependencies +node_modules +/.pnp +.pnp.js + +# testing +/coverage + +# misc +.DS_Store +.env.local +.env.development.local +.env.test.local +.env.production.local + +npm-debug.log* +yarn-debug.log* +yarn-error.log* + +packages/**/public +/dist diff --git a/__fixtures__/source-type/.gitignore b/__fixtures__/source-type/.gitignore new file mode 100644 index 000000000..d3f6241cb --- /dev/null +++ b/__fixtures__/source-type/.gitignore @@ -0,0 +1,24 @@ +# See https://help.github.com/articles/ignoring-files/ for more about ignoring files. + +# dependencies +node_modules +/.pnp +.pnp.js + +# testing +/coverage + +# misc +.DS_Store +.env.local +.env.development.local +.env.test.local +.env.production.local + +npm-debug.log* +yarn-debug.log* +yarn-error.log* + +/dist + +.vscode diff --git a/__fixtures__/source-type/.prettierignore b/__fixtures__/source-type/.prettierignore new file mode 100644 index 000000000..f60fee050 --- /dev/null +++ b/__fixtures__/source-type/.prettierignore @@ -0,0 +1,2 @@ +/dist +/packages/**/public diff --git a/__fixtures__/source-type/.yarnrc b/__fixtures__/source-type/.yarnrc new file mode 100644 index 000000000..67d13ac5f --- /dev/null +++ b/__fixtures__/source-type/.yarnrc @@ -0,0 +1 @@ +disable-self-update-check true \ No newline at end of file diff --git a/__fixtures__/source-type/README.md b/__fixtures__/source-type/README.md new file mode 100644 index 000000000..28ac919a4 --- /dev/null +++ b/__fixtures__/source-type/README.md @@ -0,0 +1 @@ +This is the `README.md` for the whole monorepo. diff --git a/__fixtures__/source-type/modular/setupEnvironment.ts b/__fixtures__/source-type/modular/setupEnvironment.ts new file mode 100644 index 000000000..5b785ac2b --- /dev/null +++ b/__fixtures__/source-type/modular/setupEnvironment.ts @@ -0,0 +1,2 @@ +// Allows for adding setup configuration to Jest +export {}; diff --git a/__fixtures__/source-type/modular/setupTests.ts b/__fixtures__/source-type/modular/setupTests.ts new file mode 100644 index 000000000..74b1a275a --- /dev/null +++ b/__fixtures__/source-type/modular/setupTests.ts @@ -0,0 +1,5 @@ +// jest-dom adds custom jest matchers for asserting on DOM nodes. +// allows you to do things like: +// expect(element).toHaveTextContent(/react/i) +// learn more: https://github.com/testing-library/jest-dom +import '@testing-library/jest-dom/extend-expect'; diff --git a/__fixtures__/source-type/package.json b/__fixtures__/source-type/package.json new file mode 100644 index 000000000..475e70a1b --- /dev/null +++ b/__fixtures__/source-type/package.json @@ -0,0 +1,60 @@ +{ + "name": "ghost-building", + "version": "1.0.0", + "main": "index.js", + "author": "Cristiano Belloni ", + "license": "MIT", + "private": true, + "workspaces": [ + "packages/**" + ], + "modular": { + "type": "root" + }, + "scripts": { + "start": "modular start", + "build": "modular build", + "test": "modular test", + "lint": "eslint . --ext .js,.ts,.tsx", + "prettier": "prettier --write ." + }, + "eslintConfig": { + "extends": "modular-app" + }, + "browserslist": { + "production": [ + ">0.2%", + "not dead", + "not op_mini all" + ], + "development": [ + "last 1 chrome version", + "last 1 firefox version", + "last 1 safari version" + ] + }, + "prettier": { + "singleQuote": true, + "trailingComma": "all", + "printWidth": 80, + "proseWrap": "always" + }, + "dependencies": { + "@testing-library/dom": "^8.19.0", + "@testing-library/jest-dom": "^5.16.5", + "@testing-library/react": "^13.4.0", + "@testing-library/user-event": "^7.2.1", + "@types/jest": "^29.2.3", + "@types/node": "^18.11.9", + "@types/react": "^18.0.25", + "@types/react-dom": "^18.0.9", + "eslint-config-modular-app": "^3.0.2", + "modular-scripts": "^3.5.0", + "modular-template-app": "^1.1.0", + "modular-template-package": "^1.1.0", + "prettier": "^2.7.1", + "react": "^18.2.0", + "react-dom": "^18.2.0", + "typescript": ">=4.2.1 <4.5.0" + } +} diff --git a/__fixtures__/source-type/packages/a/package.json b/__fixtures__/source-type/packages/a/package.json new file mode 100644 index 000000000..d5b59987c --- /dev/null +++ b/__fixtures__/source-type/packages/a/package.json @@ -0,0 +1,12 @@ +{ + "name": "a", + "private": false, + "modular": { + "type": "package" + }, + "main": "./src/index.ts", + "version": "1.0.0", + "dependencies": { + "e": "1.0.0" + } +} diff --git a/__fixtures__/source-type/packages/a/src/index.ts b/__fixtures__/source-type/packages/a/src/index.ts new file mode 100644 index 000000000..b92ce9fdf --- /dev/null +++ b/__fixtures__/source-type/packages/a/src/index.ts @@ -0,0 +1,3 @@ +export default function add(a: number, b: number): number { + return a + b; +} diff --git a/__fixtures__/source-type/packages/b/package.json b/__fixtures__/source-type/packages/b/package.json new file mode 100644 index 000000000..b32e433b5 --- /dev/null +++ b/__fixtures__/source-type/packages/b/package.json @@ -0,0 +1,12 @@ +{ + "name": "b", + "private": false, + "modular": { + "type": "package" + }, + "main": "./src/index.ts", + "version": "1.0.0", + "dependencies": { + "c": "1.0.0" + } +} diff --git a/__fixtures__/source-type/packages/b/src/index.ts b/__fixtures__/source-type/packages/b/src/index.ts new file mode 100644 index 000000000..b92ce9fdf --- /dev/null +++ b/__fixtures__/source-type/packages/b/src/index.ts @@ -0,0 +1,3 @@ +export default function add(a: number, b: number): number { + return a + b; +} diff --git a/__fixtures__/source-type/packages/c/package.json b/__fixtures__/source-type/packages/c/package.json new file mode 100644 index 000000000..47d3e287f --- /dev/null +++ b/__fixtures__/source-type/packages/c/package.json @@ -0,0 +1,13 @@ +{ + "name": "c", + "private": false, + "modular": { + "type": "source" + }, + "main": "./src/index.ts", + "version": "1.0.0", + "dependencies": { + "b": "1.0.0", + "d": "1.0.0" + } +} diff --git a/__fixtures__/source-type/packages/c/src/index.ts b/__fixtures__/source-type/packages/c/src/index.ts new file mode 100644 index 000000000..b92ce9fdf --- /dev/null +++ b/__fixtures__/source-type/packages/c/src/index.ts @@ -0,0 +1,3 @@ +export default function add(a: number, b: number): number { + return a + b; +} diff --git a/__fixtures__/source-type/packages/d/package.json b/__fixtures__/source-type/packages/d/package.json new file mode 100644 index 000000000..ec95e8355 --- /dev/null +++ b/__fixtures__/source-type/packages/d/package.json @@ -0,0 +1,9 @@ +{ + "name": "d", + "private": false, + "modular": { + "type": "source" + }, + "main": "./src/index.ts", + "version": "1.0.0" +} diff --git a/__fixtures__/source-type/packages/d/src/index.ts b/__fixtures__/source-type/packages/d/src/index.ts new file mode 100644 index 000000000..b92ce9fdf --- /dev/null +++ b/__fixtures__/source-type/packages/d/src/index.ts @@ -0,0 +1,3 @@ +export default function add(a: number, b: number): number { + return a + b; +} diff --git a/__fixtures__/source-type/packages/e/package.json b/__fixtures__/source-type/packages/e/package.json new file mode 100644 index 000000000..552c61108 --- /dev/null +++ b/__fixtures__/source-type/packages/e/package.json @@ -0,0 +1,12 @@ +{ + "name": "e", + "private": false, + "modular": { + "type": "package" + }, + "main": "./src/index.ts", + "version": "1.0.0", + "dependencies": { + "a": "1.0.0" + } +} diff --git a/__fixtures__/source-type/packages/e/src/index.ts b/__fixtures__/source-type/packages/e/src/index.ts new file mode 100644 index 000000000..b92ce9fdf --- /dev/null +++ b/__fixtures__/source-type/packages/e/src/index.ts @@ -0,0 +1,3 @@ +export default function add(a: number, b: number): number { + return a + b; +} diff --git a/__fixtures__/source-type/tsconfig.json b/__fixtures__/source-type/tsconfig.json new file mode 100644 index 000000000..d2e2dbe0a --- /dev/null +++ b/__fixtures__/source-type/tsconfig.json @@ -0,0 +1,4 @@ +{ + "extends": "modular-scripts/tsconfig.json", + "include": ["modular", "packages/**/src"] +} diff --git a/docs/commands/add.md b/docs/commands/add.md index 1bb7ad9a1..9ea6ed55c 100644 --- a/docs/commands/add.md +++ b/docs/commands/add.md @@ -28,12 +28,18 @@ Packages can currently be one of the following types: architecture or started as a normal standalone application. See also [the view building reference](../esm-views/index.md) -- A `view`, which is a package that exports a React component by default. Read +- A `view`, which is a `package` that exports a React component by default. Read more about Views in [this explainer](../concepts/views.md). -- A generic JavaScript `package`. You can use this to create any other kind of - utility, tool, or whatever your needs require you to do. As an example, you - could build a node.js server inside one of these. +- A generic JavaScript `package`. You can use this to create a library with an + entry point that gets transpiled to Common JS and ES Module format when built. + Packages can be [built](../commands/build.md) but not + [start](../commands/start.md)ed by Modular. + +- A `source`, which is a shared package that is imported by other packages from + source (i.e. directly importing its source), and it's never built standalone + or published. This kind of package is never [built](../commands/build.md) or + [start](../commands/start.md)ed by Modular. ## Options: diff --git a/docs/concepts/circular-dependencies.md b/docs/concepts/circular-dependencies.md new file mode 100644 index 000000000..f85a9e19a --- /dev/null +++ b/docs/concepts/circular-dependencies.md @@ -0,0 +1,78 @@ +--- +title: Circular Dependencies +parent: Concepts +--- + +# Circular dependencies and Modular + +`modular build` always tries to calculate the build order based on the workspace +dependency graph. If it encounters a cyclic dependency during this calculation, +it will bail out, since the build order can't be determined (if A depends on the +build result of B and B depends on the build result of A, which one should be +get built first?) + +Circular dependencies should never be introduced in your Modular monorepository; +apart from interfering with the calculation of build order, they can +[lead to unexpected results in require order](https://nodejs.org/api/modules.html#cycles), +they can confuse developer tools and they are always fixable +[by creating additional dependencies which contain the common parts](https://nx.dev/recipes/other/resolve-circular-dependencies). +Even you manage to avoid all of those issues, circular dependencies can make +refactoring fragile: if a part of A depends on a part of B and an unrelated part +of B depends on an unrelated part of A, refactoring code in A can make the +require order in B fail and vice versa. + +## `--dangerouslyIgnoreCircularDependencies` escape hatch + +If your circular dependencies involve packages that never get built (namely, +`modular.type: source` packages), Modular can still calculate the correct build +order by removing them from the dependency graph. By default, `modular build` +will still refuse to build any build graph that contains a circular dependency, +but this behaviour can be overridden by specifying the +`--dangerouslyIgnoreCircularDependencies` flag. Please note that `modular build` +will still fail if the cycle involves two or more buildable (i.e. non-`source`) +packages. This doesn't solve all the other issues linked to +`--dangerouslyIgnoreCircularDependencies`, so please don't use this flag in +production and always split you dependencies to avoid cycles. + +## Examples + +### Cycle disappearing when `source` types are removed from the dependency graph (cycle between `package` and `source`) + +(assuming that `package` b is depending on `source` c and `source` c is +depending on `package` b and `package` d): + +#### Without `--dangerouslyIgnoreCircularDependencies` the build fails + +```bash +> modular-dev build b --descendants +[modular] Building packages at: b +[modular] Cycle detected, b -> c -> b +``` + +#### With `--dangerouslyIgnoreCircularDependencies` the build warns but succeeds + +```bash +> modular-dev build b --descendants --dangerouslyIgnoreCircularDependencies +[modular] Building packages at: b +[modular] You chose to dangerously ignore cycles in the dependency graph. Builds will still fail if a cycle is found involving two or more buildable packages. Please note that the use of this flag is not recommended. It's always possible to break a cyclic dependency by creating an additional dependency that contains the common code. +[modular] $ b: generating .d.ts files +[modular] $ b: building b... +[modular] $ b: built b in /Users/N761472/dev/rig/ghost-building/dist/b +[modular] $ d: generating .d.ts files +[modular] $ d: building d... +[modular] $ d: built d in /Users/N761472/dev/rig/ghost-building/dist/d +``` + +### Cycle not disappearing when `source` types are removed from the dependency graph (cycle between `package`s) + +(assuming that `package` b is depending on `package` c and `package` c is +depending on `package` b and `package` d): + +#### Even with `--dangerouslyIgnoreCircularDependencies` the build fails: + +```bash +> modular-dev build b --descendants --dangerouslyIgnoreCircularDependencies +[modular] Building packages at: b +[modular] You chose to dangerously ignore cycles in the dependency graph. Builds will still fail if a cycle is found involving two or more buildable packages. Please note that the use of this flag is not recommended. It's always possible to break a cyclic dependency by creating an additional dependency that contains the common code. +[modular] Cycle detected, b -> c -> b +``` diff --git a/packages/modular-scripts/src/__tests__/selectiveBuild.test.ts b/packages/modular-scripts/src/__tests__/selectiveBuild.test.ts index b9fda2782..3cbe9522e 100644 --- a/packages/modular-scripts/src/__tests__/selectiveBuild.test.ts +++ b/packages/modular-scripts/src/__tests__/selectiveBuild.test.ts @@ -60,7 +60,7 @@ describe('--changed builds all the changed packages in order', () => { '--changed', ]); expect(result.stderr).toBeFalsy(); - expect(result.stdout).toContain('No changed workspaces found'); + expect(result.stdout).toContain('No workspaces to build'); }); it('builds multiple packages', () => { diff --git a/packages/modular-scripts/src/__tests__/sourceType.test.ts b/packages/modular-scripts/src/__tests__/sourceType.test.ts new file mode 100644 index 000000000..c5989c734 --- /dev/null +++ b/packages/modular-scripts/src/__tests__/sourceType.test.ts @@ -0,0 +1,68 @@ +import execa from 'execa'; +import path from 'path'; +import fs from 'fs-extra'; + +import getModularRoot from '../utils/getModularRoot'; +import { createModularTestContext, runLocalModular } from '../test/utils'; + +// Temporary test context paths set by createTempModularRepoWithTemplate() +let tempModularRepo: string; + +const currentModularFolder = getModularRoot(); + +describe('source types and circular dependencies', () => { + const fixturesFolder = path.join( + getModularRoot(), + '__fixtures__', + 'source-type', + ); + + beforeAll(() => { + tempModularRepo = createModularTestContext(); + fs.copySync(fixturesFolder, tempModularRepo); + execa.sync('yarn', { + cwd: tempModularRepo, + }); + execa.sync('git', ['init'], { + cwd: tempModularRepo, + }); + }); + + it('when building a source type, command succeeds but nothing is built', () => { + const result = runLocalModular(currentModularFolder, tempModularRepo, [ + 'build', + 'd', + ]); + expect(result.stderr).toBeFalsy(); + expect(result.stdout).toContain('No workspaces to build'); + }); + + it('when building a package that has a circular dependency, command fails', () => { + expect(() => + runLocalModular(currentModularFolder, tempModularRepo, ['build', 'b']), + ).toThrow('Cycle detected, b -> c -> b'); + }); + + it('when building a package that has a circular dependency with the --dangerouslyIgnoreCircularDependencies flag set, command warns and succeeds if removing the source package from the graph removes the cycle', () => { + const result = runLocalModular(currentModularFolder, tempModularRepo, [ + 'build', + 'b', + '--dangerouslyIgnoreCircularDependencies', + ]); + + expect(result.stderr).toContain( + 'You chose to dangerously ignore cycles in the dependency graph', + ); + expect(result.stdout).toContain('built b in'); + }); + + it('when building a package that has a circular dependency with the --dangerouslyIgnoreCircularDependencies flag set, command still fails if removing the source package from the graph does not remove the cycle', () => { + expect(() => + runLocalModular(currentModularFolder, tempModularRepo, [ + 'build', + 'a', + '--dangerouslyIgnoreCircularDependencies', + ]), + ).toThrow('Cycle detected, e -> a -> e'); + }); +}); diff --git a/packages/modular-scripts/src/__tests__/utils/__snapshots__/getWorkspaceInfo.test.ts.snap b/packages/modular-scripts/src/__tests__/utils/__snapshots__/getWorkspaceInfo.test.ts.snap index 154497336..f423d1438 100644 --- a/packages/modular-scripts/src/__tests__/utils/__snapshots__/getWorkspaceInfo.test.ts.snap +++ b/packages/modular-scripts/src/__tests__/utils/__snapshots__/getWorkspaceInfo.test.ts.snap @@ -93,7 +93,7 @@ Object { exports[`getWorkspaceInfo 9`] = ` Object { - "location": "packages/modular-template-view", + "location": "packages/modular-template-source", "mismatchedWorkspaceDependencies": Array [], "public": true, "type": "template", @@ -103,6 +103,17 @@ Object { `; exports[`getWorkspaceInfo 10`] = ` +Object { + "location": "packages/modular-template-view", + "mismatchedWorkspaceDependencies": Array [], + "public": true, + "type": "template", + "version": Any, + "workspaceDependencies": Array [], +} +`; + +exports[`getWorkspaceInfo 11`] = ` Object { "location": "packages/modular-types", "mismatchedWorkspaceDependencies": Array [], @@ -113,7 +124,7 @@ Object { } `; -exports[`getWorkspaceInfo 11`] = ` +exports[`getWorkspaceInfo 12`] = ` Object { "location": "packages/modular-views.macro", "mismatchedWorkspaceDependencies": Array [], @@ -124,7 +135,7 @@ Object { } `; -exports[`getWorkspaceInfo 12`] = ` +exports[`getWorkspaceInfo 13`] = ` Object { "location": "packages/tree-view-for-tests", "mismatchedWorkspaceDependencies": Array [], @@ -135,7 +146,7 @@ Object { } `; -exports[`getWorkspaceInfo 13`] = ` +exports[`getWorkspaceInfo 14`] = ` Object { "location": "packages/workspace-resolver", "mismatchedWorkspaceDependencies": Array [], diff --git a/packages/modular-scripts/src/addPackage.ts b/packages/modular-scripts/src/addPackage.ts index 1c02a96c7..3c39613d3 100644 --- a/packages/modular-scripts/src/addPackage.ts +++ b/packages/modular-scripts/src/addPackage.ts @@ -72,6 +72,11 @@ async function promptForType(type: string | void) { value: 'esm-view', }, { title: 'A micro-frontend React container (app)', value: 'app' }, + { + title: + 'A package containing shared source code which is imported by other packages (source)', + value: 'source', + }, { title: 'Choose my own template', value: CUSTOM_TEMPLATE }, ], initial: 0, @@ -157,10 +162,14 @@ async function addPackage({ try { logger.log(`Looking for template ${templateName} in project...`); + console.log(`require.resolve(${installedPackageJsonPath}, { + paths: [${modularRoot}], + });`); templatePackageJsonPath = require.resolve(installedPackageJsonPath, { paths: [modularRoot], }); } catch (e) { + console.log(e); logger.log( `Installing template package ${templateName} from registry, this may take a moment...`, ); diff --git a/packages/modular-scripts/src/build/index.ts b/packages/modular-scripts/src/build/index.ts index e6b0bc72f..ebdfc7160 100644 --- a/packages/modular-scripts/src/build/index.ts +++ b/packages/modular-scripts/src/build/index.ts @@ -9,10 +9,10 @@ import type { ModularType } from '@modular-scripts/modular-types'; import * as logger from '../utils/logger'; import getModularRoot from '../utils/getModularRoot'; import actionPreflightCheck from '../utils/actionPreflightCheck'; -import { getModularType } from '../utils/isModularType'; +import { getModularType } from '../utils/packageTypes'; import execAsync from '../utils/execAsync'; import getLocation from '../utils/getLocation'; -import { selectWorkspaces } from '../utils/selectWorkspaces'; +import { selectBuildableWorkspaces } from '../utils/selectWorkspaces'; import { setupEnvForDirectory } from '../utils/setupEnv'; import createPaths from '../utils/createPaths'; import printHostingInstructions from './printHostingInstructions'; @@ -276,6 +276,7 @@ async function build({ descendants, changed, compareBranch, + dangerouslyIgnoreCircularDependencies, }: { packagePaths: string[]; preserveModules: boolean; @@ -284,17 +285,19 @@ async function build({ descendants: boolean; changed: boolean; compareBranch?: string; + dangerouslyIgnoreCircularDependencies: boolean; }): Promise { - const selectedTargets = await selectWorkspaces({ + const selectedTargets = await selectBuildableWorkspaces({ targets, changed, compareBranch, descendants, ancestors, + dangerouslyIgnoreCircularDependencies, }); if (!selectedTargets.length) { - logger.log('No changed workspaces found'); + logger.log('No workspaces to build'); process.exit(0); } diff --git a/packages/modular-scripts/src/check/verifyWorkspaceDependencies.ts b/packages/modular-scripts/src/check/verifyWorkspaceDependencies.ts index ba4b4e5c0..634584855 100644 --- a/packages/modular-scripts/src/check/verifyWorkspaceDependencies.ts +++ b/packages/modular-scripts/src/check/verifyWorkspaceDependencies.ts @@ -1,12 +1,9 @@ -import * as path from 'path'; -import getModularRoot from '../utils/getModularRoot'; import getWorkspaceInfo from '../utils/getWorkspaceInfo'; -import { getModularType, isValidModularType } from '../utils/isModularType'; +import { isValidModularType } from '../utils/packageTypes'; import * as logger from '../utils/logger'; export async function check(target?: string): Promise { let valid = true; - const modularRoot = getModularRoot(); // ensure that workspaces are setup correctly with yarn // init is a special case where we don't already need to be in a modular repository // in this case there's no use checking the workspaces yet because we're setting @@ -26,13 +23,11 @@ export async function check(target?: string): Promise { valid = false; } - if (!isValidModularType(path.join(modularRoot, packageInfo.location))) { + if (!isValidModularType(packageInfo.type)) { logger.error( `${packageName} at ${ packageInfo.location - } is not a valid modular type - Found ${ - getModularType(packageInfo.location) as string - }`, + } is not a valid modular type - Found ${packageInfo.type as string}`, ); valid = false; } diff --git a/packages/modular-scripts/src/program.ts b/packages/modular-scripts/src/program.ts index a5f03a380..3c40bb0fb 100755 --- a/packages/modular-scripts/src/program.ts +++ b/packages/modular-scripts/src/program.ts @@ -103,6 +103,11 @@ program '--compareBranch ', "Specifies the branch to use with the --changed flag. If not specified, Modular will use the repo's default branch", ) + .option( + '--dangerouslyIgnoreCircularDependencies', + "Ignore circular dependency checks if your graph has one or more circular dependencies involving 'source' types, then warn. The build will still fail if circular dependencies involve more than one buildable package. Circular dependencies can be always refactored to remove cycles. This switch is dangerous and should be used sparingly and only temporarily.", + false, + ) .action( async ( packagePaths: string[], @@ -113,6 +118,7 @@ program compareBranch?: string; ancestors: boolean; descendants: boolean; + dangerouslyIgnoreCircularDependencies: boolean; }, ) => { const { default: build } = await import('./build'); @@ -133,6 +139,14 @@ program process.exit(1); } + if (options.dangerouslyIgnoreCircularDependencies) { + // Warn. Users should never use this, but if they use it, they should have cycles limited to "source" packages + // and they should do this in a temporary way (for example, to onboard large projects). + logger.warn( + `You chose to dangerously ignore cycles in the dependency graph. Builds will still fail if a cycle is found involving two or more buildable packages. Please note that the use of this flag is not recommended. It's always possible to break a cyclic dependency by creating an additional dependency that contains the common code.`, + ); + } + await build({ packagePaths, preserveModules: JSON.parse(options.preserveModules) as boolean, @@ -141,6 +155,8 @@ program compareBranch: options.compareBranch, ancestors: options.ancestors, descendants: options.descendants, + dangerouslyIgnoreCircularDependencies: + options.dangerouslyIgnoreCircularDependencies, }); }, ); diff --git a/packages/modular-scripts/src/serve.ts b/packages/modular-scripts/src/serve.ts index 8219cf1f8..9a85fda18 100644 --- a/packages/modular-scripts/src/serve.ts +++ b/packages/modular-scripts/src/serve.ts @@ -5,7 +5,7 @@ import actionPreflightCheck from './utils/actionPreflightCheck'; import createPaths from './utils/createPaths'; import getLocation from './utils/getLocation'; import * as logger from './utils/logger'; -import isModularType from './utils/isModularType'; +import { isModularType } from './utils/packageTypes'; async function serve(target: string, port = 3000): Promise { const targetLocation = await getLocation(target); diff --git a/packages/modular-scripts/src/start.ts b/packages/modular-scripts/src/start.ts index f592588f6..ca2d6e19b 100644 --- a/packages/modular-scripts/src/start.ts +++ b/packages/modular-scripts/src/start.ts @@ -1,7 +1,11 @@ import { paramCase as toParamCase } from 'change-case'; import actionPreflightCheck from './utils/actionPreflightCheck'; -import isModularType from './utils/isModularType'; +import { + isModularType, + getModularType, + isStartableModularType, +} from './utils/packageTypes'; import execAsync from './utils/execAsync'; import getLocation from './utils/getLocation'; import stageView from './utils/stageView'; @@ -16,6 +20,7 @@ import createEsbuildBrowserslistTarget from './utils/createEsbuildBrowserslistTa import prompts from 'prompts'; import { getDependencyInfo } from './utils/getDependencyInfo'; import { isReactNewApi } from './utils/isReactNewApi'; +import type { PackageType } from '@modular-scripts/modular-types'; async function start(packageName: string): Promise { let target = packageName; @@ -40,9 +45,14 @@ async function start(packageName: string): Promise { await setupEnvForDirectory(targetPath); - if (isModularType(targetPath, 'package')) { + const modularType = getModularType(targetPath); + if (!modularType || !isStartableModularType(modularType as PackageType)) { throw new Error( - `The package at ${targetPath} is not a valid modular app or view.`, + `The package at ${targetPath} can't be started because ${ + modularType + ? `has Modular type "${modularType}"` + : `has no Modular type` + }.`, ); } diff --git a/packages/modular-scripts/src/utils/isModularType.ts b/packages/modular-scripts/src/utils/isModularType.ts deleted file mode 100644 index 3a492885b..000000000 --- a/packages/modular-scripts/src/utils/isModularType.ts +++ /dev/null @@ -1,41 +0,0 @@ -import * as path from 'path'; - -import type { - ModularType, - PackageType, - ModularPackageJson, -} from '@modular-scripts/modular-types'; - -import * as fs from 'fs-extra'; - -export const packageTypes: PackageType[] = [ - 'app', - 'esm-view', - 'view', - 'package', - 'template', -]; -export const ModularTypes: ModularType[] = ( - packageTypes as ModularType[] -).concat(['root']); - -export function getModularType(dir: string): ModularType | undefined { - const packageJsonPath = path.join(dir, 'package.json'); - if (fs.existsSync(packageJsonPath)) { - const packageJson = fs.readJsonSync(packageJsonPath) as ModularPackageJson; - return packageJson.modular?.type || 'package'; - } -} - -export default function isModularType(dir: string, type: PackageType): boolean { - const packageJsonPath = path.join(dir, 'package.json'); - if (fs.existsSync(packageJsonPath)) { - const packageJson = fs.readJsonSync(packageJsonPath) as ModularPackageJson; - return packageJson.modular?.type === type; - } - return false; -} - -export function isValidModularType(dir: string): boolean { - return ModularTypes.includes(getModularType(dir) as ModularType); -} diff --git a/packages/modular-scripts/src/utils/packageTypes.ts b/packages/modular-scripts/src/utils/packageTypes.ts new file mode 100644 index 000000000..148b447e5 --- /dev/null +++ b/packages/modular-scripts/src/utils/packageTypes.ts @@ -0,0 +1,60 @@ +import path from 'path'; +import fs from 'fs-extra'; + +import type { + ModularType, + PackageType, + ModularPackageJson, +} from '@modular-scripts/modular-types'; + +interface PackageTypeDefinition { + buildable: boolean; + testable: boolean; + startable: boolean; +} + +type PackageTypeDefinitions = { [Type in PackageType]: PackageTypeDefinition }; + +const packageTypeDefinitions: PackageTypeDefinitions = { + app: { buildable: true, testable: true, startable: true }, + 'esm-view': { buildable: true, testable: true, startable: true }, + view: { buildable: true, testable: true, startable: true }, + package: { buildable: true, testable: true, startable: false }, + template: { buildable: false, testable: false, startable: false }, + source: { buildable: false, testable: true, startable: false }, +}; + +export const packageTypes = Object.keys(packageTypeDefinitions); + +export const ModularTypes: ModularType[] = ( + packageTypes as ModularType[] +).concat(['root']); + +export function getModularType(dir: string): ModularType | undefined { + const packageJsonPath = path.join(dir, 'package.json'); + if (fs.existsSync(packageJsonPath)) { + const packageJson = fs.readJsonSync(packageJsonPath) as ModularPackageJson; + return packageJson.modular?.type; + } +} + +export function isModularType(dir: string, type: PackageType): boolean { + const packageJsonPath = path.join(dir, 'package.json'); + if (fs.existsSync(packageJsonPath)) { + const packageJson = fs.readJsonSync(packageJsonPath) as ModularPackageJson; + return packageJson.modular?.type === type; + } + return false; +} + +export function isValidModularType(type: string): boolean { + return ModularTypes.includes(type as ModularType); +} + +export function isBuildableModularType(type: PackageType): boolean { + return packageTypeDefinitions[type].buildable; +} + +export function isStartableModularType(type: PackageType): boolean { + return packageTypeDefinitions[type].startable; +} diff --git a/packages/modular-scripts/src/utils/selectWorkspaces.ts b/packages/modular-scripts/src/utils/selectWorkspaces.ts index 7ad689809..5bd8347cb 100644 --- a/packages/modular-scripts/src/utils/selectWorkspaces.ts +++ b/packages/modular-scripts/src/utils/selectWorkspaces.ts @@ -3,13 +3,14 @@ import { computeAncestorSet, traverseWorkspaceRelations, } from '@modular-scripts/workspace-resolver'; - +import { isBuildableModularType } from './packageTypes'; import { getAllWorkspaces } from './getAllWorkspaces'; import { getChangedWorkspacesContent } from './getChangedWorkspaces'; import getModularRoot from './getModularRoot'; +import { PackageType, WorkspaceMap } from '@modular-scripts/modular-types'; /** - * @typedef {Object} TargetOptions + * @typedef {Object} SelectOptions * @property {string[]} targets - An array of package names to select * @property {boolean} changed - Whether to additionally select packages that have changes, compared to "compareBranch" * @property {boolean} ancestors - Whether to additionally select packages that are ancestors of (i.e.: [in]directly depend on) the selected packages @@ -17,13 +18,7 @@ import getModularRoot from './getModularRoot'; * @property {string?} compareBranch - The git branch to compare with when "changed" is specified */ -/** - * Select target packages in workspaces, optionally including changed, ancestors and descendant packages - * @param {TargetOptions} name The target options to configure selection - * @return {Promise} A distinct list of selected package names - */ - -interface TargetOptions { +interface SelectOptions { targets: string[]; changed: boolean; ancestors: boolean; @@ -31,51 +26,89 @@ interface TargetOptions { compareBranch?: string; } -export async function selectWorkspaces({ - targets, - changed, - ancestors, - descendants, - compareBranch, -}: TargetOptions): Promise { - const [, allWorkspacesMap] = await getAllWorkspaces(getModularRoot()); - let changedTargets: string[] = []; - - if (changed) { - const [, buildTargetMap] = await getChangedWorkspacesContent(compareBranch); - changedTargets = Object.keys(buildTargetMap); - } +interface SelectBuildableOptions extends SelectOptions { + dangerouslyIgnoreCircularDependencies?: boolean; +} - const targetsToBuild = [...new Set(targets.concat(changedTargets))]; +/** + * Select all target packages in workspaces in random order, optionally including changed, ancestors and descendant packages + * + * This method will not throw, even if there are circular dependencies in the graph + * + * @param {SelectOptions} options The target options to configure selection + * @return {Promise} A distinct list of selected package names + */ - if (!targetsToBuild.length) { - return []; - } +export async function selectWorkspaces( + options: SelectOptions, +): Promise { + return [...new Set((await computeWorkspaceSelection(options)).packageScope)]; +} - // Calculate the package scope, i.e. the deduped array of all the packages we need to generate an ordering graph for. - // We need to remember ancestor and descendant sets in order to select them later. - let ancestorsSet: Set = new Set(); - let descendantsSet: Set = new Set(); - let packageScope = targetsToBuild; +/** + * Select buildable target packages in workspaces in build order, optionally including changed, ancestors and descendant packages + * + * Please note that the build order algorithm can't calculate build order if there's a cycle in the dependency graph, + * so circular dependencies will throw unless they only involve packages that don't get built (i.e. "source" `modular.type`s). + * Please also note that circular dependencies can always be fixed by creating an additional package that contains the common parts, + * and that they are considered a code smell + a source of many issues (e.g. https://nodejs.org/api/modules.html#modules_cycles) + * and they make your code fragile towards refactoring, so please don't introduce them in your monorepository. + * + * @param {SelectBuildableOptions} options The target options to configure selection + * @return {Promise} A distinct list of selected buildable package names, in build order + */ - if (descendants) { - descendantsSet = computeDescendantSet(targetsToBuild, allWorkspacesMap); - packageScope = packageScope.concat([...descendantsSet]); +export async function selectBuildableWorkspaces( + options: SelectBuildableOptions, +): Promise { + const { ancestors, descendants, dangerouslyIgnoreCircularDependencies } = + options; + const { + packageScope, + isBuildable, + allWorkspacesMap, + descendantsSet, + ancestorsSet, + targetsToBuild, + } = await computeWorkspaceSelection(options); + if (!dangerouslyIgnoreCircularDependencies) { + // Here we're using traverseWorkspaceRelations to warn us if there's at least one cyclical dependency in the graph + // Since the dependency graph can be disconnected, it makes only sense to calculate cycles in the context + // of a subset of packages we want to build (aka the package scope) + void traverseWorkspaceRelations(packageScope, allWorkspacesMap); } - if (ancestors) { - ancestorsSet = computeAncestorSet(targetsToBuild, allWorkspacesMap); - packageScope = packageScope.concat([...ancestorsSet]); - } + const buildablePackageScope = [...new Set(packageScope)].filter(isBuildable); + // Since buildOrder is true, we want to select packages in build order. The build order algorithm gives up if there's a cycle in the dependency graph + // (since it can't know what to build first if A depends on B but B depends on A), so we can allow cycles only if they involve packages that are not built + // (i.e. "source" `modular.type`s). Please note that dependency cycles can always be fixed by creating an additional package that contains the common parts, + // and that circular dependencies are a source of many additional issues. - packageScope = [...new Set(packageScope)]; + // The package graph is reduced to a graph where all vertices are buildable. + // To do that, we need to first filter the vertices, then filter the edges that might be pointing to a non-buildable vertex + const buildableWorkspacesMap = Object.fromEntries( + Object.entries(allWorkspacesMap) + .filter(([name]) => isBuildable(name)) + .map(([name, workspace]) => { + return [ + name, + { + ...workspace, + workspaceDependencies: + workspace.workspaceDependencies.filter(isBuildable), + }, + ]; + }), + ); - // traverseWorkspaceRelations will walk the graph and generate the whole ordering needed to build a package + // traverseWorkspaceRelations will walk the given graph (in this case, the graph of all buildable packages) and generate the whole ordering needed to build a package // which means that it will possibly expand the scope (it just generates the dependency order starting from a dependency subset but taking into account the whole dependency graph) - // this means that we can build in order even manually selected subsets of packages (like: "the user wants to select only a and b, but execute tasks on them in order"), - // but it also means we need to filter out all the unwanted packages later. + // this means that we can generate an order from any strict subset of packages, but we need to filter out all the unwanted packages later. const targetEntriesWithOrder = [ - ...traverseWorkspaceRelations(packageScope, allWorkspacesMap).entries(), + ...traverseWorkspaceRelations( + buildablePackageScope, + buildableWorkspacesMap, + ).entries(), ]; return ( @@ -92,3 +125,69 @@ export async function selectWorkspaces({ ) ); } + +/** + * Common data structures to calculate package selection + */ +async function computeWorkspaceSelection({ + targets, + changed, + ancestors, + descendants, + compareBranch, +}: SelectOptions): Promise<{ + packageScope: string[]; + isBuildable: (name: string) => boolean | undefined; + allWorkspacesMap: WorkspaceMap; + ancestorsSet: Set; + descendantsSet: Set; + targetsToBuild: string[]; +}> { + const [allWorkspacePackages, allWorkspacesMap] = await getAllWorkspaces( + getModularRoot(), + ); + let changedTargets: string[] = []; + + if (changed) { + const [, buildTargetMap] = await getChangedWorkspacesContent(compareBranch); + changedTargets = Object.keys(buildTargetMap); + } + + const targetsToBuild = [...new Set(targets.concat(changedTargets))]; + + // We want to remove all the non-buildable packages from the package scope. Create a filter predicate that looks up to the package map + const isBuildable = (name: string) => { + const type = allWorkspacePackages.get(name)?.modular?.type; + return type && isBuildableModularType(type as PackageType); + }; + + let ancestorsSet: Set = new Set(); + let descendantsSet: Set = new Set(); + let packageScope: string[] = []; + + if (targetsToBuild.length) { + // Calculate the package scope, i.e. the deduped array of all the packages we need to generate an ordering graph for. + // We need to remember ancestor and descendant sets in order to select them later. + + packageScope = targetsToBuild; + + if (descendants) { + descendantsSet = computeDescendantSet(targetsToBuild, allWorkspacesMap); + packageScope = packageScope.concat([...descendantsSet]); + } + + if (ancestors) { + ancestorsSet = computeAncestorSet(targetsToBuild, allWorkspacesMap); + packageScope = packageScope.concat([...ancestorsSet]); + } + } + + return { + packageScope, + isBuildable, + allWorkspacesMap, + ancestorsSet, + descendantsSet, + targetsToBuild, + }; +} diff --git a/packages/modular-template-source/package.json b/packages/modular-template-source/package.json new file mode 100644 index 000000000..851b1402a --- /dev/null +++ b/packages/modular-template-source/package.json @@ -0,0 +1,17 @@ +{ + "name": "modular-template-source", + "version": "1.0.0", + "exports": { + "./package.json": "./package.json" + }, + "main": "./src/index.ts", + "modular": { + "type": "template", + "templateType": "source" + }, + "license": "Apache-2.0", + "files": [ + "README.md", + "src" + ] +} diff --git a/packages/modular-template-source/src/__tests__/index.test.ts b/packages/modular-template-source/src/__tests__/index.test.ts new file mode 100644 index 000000000..62824a092 --- /dev/null +++ b/packages/modular-template-source/src/__tests__/index.test.ts @@ -0,0 +1,5 @@ +import add from '../index'; + +test('it should add two numbers', () => { + expect(add(0.1, 0.2)).toEqual(0.30000000000000004); +}); diff --git a/packages/modular-template-source/src/index.ts b/packages/modular-template-source/src/index.ts new file mode 100644 index 000000000..b92ce9fdf --- /dev/null +++ b/packages/modular-template-source/src/index.ts @@ -0,0 +1,3 @@ +export default function add(a: number, b: number): number { + return a + b; +} diff --git a/packages/modular-types/src/types.ts b/packages/modular-types/src/types.ts index e40f0dc3e..321dfc81c 100644 --- a/packages/modular-types/src/types.ts +++ b/packages/modular-types/src/types.ts @@ -1,6 +1,11 @@ import type { JSONSchemaForNPMPackageJsonFiles as PackageJson } from '@schemastore/package'; -export type ModularTemplateType = 'app' | 'esm-view' | 'view' | 'package'; +export type ModularTemplateType = + | 'app' + | 'esm-view' + | 'view' + | 'package' + | 'source'; export type PackageType = ModularTemplateType | 'template'; export type UnknownType = 'unknown';