Generate paths for modular add by cristiano-belloni · Pull Request #2111 · jpmorganchase/modular

cristiano-belloni · 2022-09-06T12:35:21Z

TODO

Revised proposition with use-cases:

When a user wants to add an unscoped package to the packages root

# adds "pkg" in ./packages/pkg
modular add PKG

When a user wants to add an unscoped package to a nested path

# adds "pkg" in packages/my/nested/path/pkg
modular add pkg --path packages/my/nested/path

When a user wants to add a scoped package

# adds "@scope/pkg" in packages/pkg
modular add @scope/pkg

When a user wants to add a scoped package to a nested path

# adds "@scope/pkg" in packages/my/nested/path/scope/pkg
modular add @scope/pkg --path packages/my/nested/path/scope

When a user wants to add a package (scoped or otherwise) to a path other than ./packages

# adds "pkg" in my/other/path/pkg
modular add pkg --path /my/other/path

# adds "@scope/pkg" in my/other/path/pkg
modular add @scope/PKG --path my/other/path

Validity checks (from @steveukx 's comment):

Assuming --path contains a relative directory (would need to validate doesn't use .. blocks to jump outside of repo root) that can be used in place of the default ./packages.
Caveat of using a --path, modular should check that the workspace would find the newly created package with the workspaces property in root package.json

Additional checks

Modular now checks for the existence of a package with the same name, using the workspace resolver. Before, if a workspace with the same name already existed, Modular would still add a new one, but Yarn would error at the end of the command, giving the impression that the command failed. Now the command fails straight away.
Modular now checks for the existence of the target directory and fail if the directory is not empty. Before, it just silently wrote in the existing directory without clearing it first (potentially overwriting some, but not necessarily all, files).

changeset-bot · 2022-09-06T12:35:26Z

🦋 Changeset detected

Latest commit: 8cdfc02

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
modular-scripts	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

coveralls · 2022-09-06T13:01:46Z

Coverage decreased (-0.5%) to 33.419% when pulling 8cdfc02 on fix/nested-package-names into e41effa on main.

benpryke · 2022-09-06T16:43:43Z

packages/modular-scripts/src/utils/parsePackageName.ts

+export const packageRegex =
+  /^(@[a-z0-9-~][a-z0-9-._~]*)?\/?([a-z0-9-~][a-z0-9-._~]*)\/?(.*)/;
+
+export function parsePackageName(name: string): {


This could do with a docstring with some examples of what each of the return values are.

benpryke · 2022-09-06T16:47:25Z

packages/modular-scripts/src/addPackage.ts

  const installedPackageJsonPath = path.join(templateName, 'package.json');
+
+  const { dependencyName: packageName, scope, module } = parsePackageName(name);
+  const { componentName, packagePath } = getNewPackageDetails({


I suggest that it would be cleaner to call getNewPackageDetails(name) and return { packageName, packagePath, componentName }. This would move the call to parsePackageName into getNewPackageDetails and avoid declaring the scope and module variables in the scope of promptForName when they are only used once.

benpryke · 2022-09-06T16:51:39Z

packages/modular-scripts/src/addPackage.ts

 }: AddOptions): Promise<void> {
  const name = await promptForName(nameArg || unstableName);
+
+  // Validate package name


This entire validation section from 146 to 174 looks like it could go into a separate function.

benpryke · 2022-09-06T16:52:49Z

packages/modular-scripts/src/addPackage.ts

  let newModularPackageJsonPath;

-  // try and find the modular template packge, if it's already been installed
+  // Find out if a package with the same name already exists


This entire validation section from 190 to 208 looks like it could also go into a separate function.

benpryke · 2022-09-06T16:54:00Z

packages/modular-scripts/src/addPackage.ts


-  // try and find the modular template packge, if it's already been installed
+  // Find out if a package with the same name already exists
+  if ((await getWorkspaceInfo())[packageName]) {


How long does this take in a large monorepo (e.g. 100-500 packages)?

@sgb-io ran benchmarks on that, and we're < 2ms for 1k workspaces.

benpryke · 2022-09-06T17:08:39Z

packages/modular-scripts/src/utils/isInWorkspaces.ts

+}
+
+// This function gets all the globs in "workspaces" field in the root package manifest
+export async function getWorkspaceGlobs(): Promise<string[]> {


As this module exports 2 different workspace-related functions, perhaps it would better be called workspaces (i.e. this is where all utils for workspaces are placed).

The second export isn't currently used in the project, so this could be private to the module

Removed the export

…napshots

sgb-io

LGTM

sgb-io · 2022-09-12T11:52:23Z

docs/commands/add.md

+# `modular add <packageName>`

-Adds a new package by creating a new workspace at `packages/<packagePath>`
+Adds a new package by creating a new workspace at `packages/<packageName>`,


How about we have the 5 examples from the discussion thread right in the docs? I think it makes it really fast to understand how to use the option (even though your written docs are also good!)

sgb-io · 2022-09-12T11:54:18Z

packages/modular-scripts/src/addPackage.ts

+  // Try and find the modular template package. If it's already been installed
  // in the project then continue without needing to do an install.
  // else we will fetch it from the yarn registry.
+  let newModularPackageJsonPath;


nit: not related to your change, but this could he instantiated with an explicit string | undefined type

Verify package names, factor out package details function with new logic

10d9329

Cristiano Belloni added 3 commits September 6, 2022 15:27

Add --path option + fix checking if package exists

cf8f15f

Check that the target path is inside modular root

be9d8da

add validity checks for new added package

b47b706

benpryke reviewed Sep 6, 2022

View reviewed changes

Cristiano Belloni and others added 11 commits September 7, 2022 14:44

Refactor validation functions

b6958b8

Add jsDoc

4d5c703

Move var in right place

25ee785

Add integration tests

4a456f7

Discard scope when creating workspace, add addPackage tests, update s…

60badd7

…napshots

Add new tests for custom directory

0ee00c0

Explicitly specify nested package in test

85f9c75

Activate addPackage tests on Windows

13532db

Update docs for add

d44b914

Improve docs

317ab86

Create hip-carrots-relax.md

b8c326e

cristiano-belloni marked this pull request as ready for review September 9, 2022 14:51

cristiano-belloni requested review from ayseenver, benpryke, sgb-io and steveukx September 9, 2022 15:19

sgb-io previously approved these changes Sep 12, 2022

View reviewed changes

Merge branch 'main' into fix/nested-package-names

578b40b

cristiano-belloni dismissed sgb-io’s stale review via 578b40b September 14, 2022 08:57

Remove double webpack

8cdfc02

sgb-io approved these changes Sep 14, 2022

View reviewed changes

cristiano-belloni merged commit deaee2c into main Sep 14, 2022

cristiano-belloni deleted the fix/nested-package-names branch September 14, 2022 10:19

This was referenced Sep 14, 2022

Version Packages #2139

Merged

Version Packages joshwooding/modular#1

Closed

Version Packages joshwooding/modular#2

Open

Conversation

cristiano-belloni commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

coveralls commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgb-io left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cristiano-belloni commented Sep 6, 2022 •

edited

Loading

changeset-bot bot commented Sep 6, 2022 •

edited

Loading

coveralls commented Sep 6, 2022 •

edited

Loading