modular rename command by cristiano-belloni · Pull Request #1163 · jpmorganchase/modular

cristiano-belloni · 2021-12-08T16:20:00Z

Done:

~~Move old package directory to new package directory~~ - We decided with @cangarugula that we don't want to do this as it's ambiguous and directory and package name are not necessarily coupled - see also docs/commands/rename.md
Change package name in package.json
~~Change dependency name in depending packages' package.json~~ - not needed.
Re-write imports in depending packages' source files
Add logs
Write tests
Add docs
Made action atomic

changeset-bot · 2021-12-08T16:20:12Z

🦋 Changeset detected

Latest commit: 40ad365

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 · 2021-12-08T16:43:11Z

Coverage decreased (-0.5%) to 28.282% when pulling 40ad365 on feature/add-rename into c25c128 on main.

…/modular into feature/add-rename

cangarugula · 2021-12-14T16:32:16Z

packages/modular-scripts/src/rename.ts

+  logger.debug(`Checking for existence of ${oldPackageName} in workspace.`);
+  const oldPackage = workspace.find(
+    ([packageName]) => packageName === oldPackageName,
+  )?.[1];


You shouldn't need to flatten this because the package names are saved as keys returned from getWorkspaceInfo that you can access via bracket notation to check if they exist in the modular workspace.

{ "app": { "location": "packages/app", "workspaceDependencies": [], "mismatchedWorkspaceDependencies": [], "type": "app", "public": false }

cangarugula · 2021-12-14T16:32:25Z

packages/modular-scripts/src/rename.ts

+    throw new Error(`Package ${oldPackageName} not found.`);
+  }
+  logger.debug(`Checking for collision with ${newPackageName} in workspace.`);
+  if (workspace.find(([packageName]) => packageName === newPackageName)) {


cangarugula · 2021-12-14T16:33:54Z

packages/modular-scripts/src/rename.ts

+
+  // Rename the directory. Do it first because this fails if there's a collision and we don't want to clean up later
+  const newPackageLocation = path.join(
+    getModularRoot(),


You call getModularRoot multiple times in this function. Can you store this at the top when you first need it and use that instead of making calls again?

@cangarugula It's memoized to allow calling it without performance penalty. I can hoist it, but everywhere else we use the "just call it" pattern, which imho is a bit more readable.

cangarugula · 2021-12-14T16:39:26Z

packages/modular-scripts/src/rename.ts

+  // Rename the directory. Do it first because this fails if there's a collision and we don't want to clean up later
+  const newPackageLocation = path.join(
+    getModularRoot(),
+    path.join(oldPackage.location, '..'),


If the newPackageName is scoped, i.e. @app/view, you'll need to create the new package location either nested respective to its scope (packages/app/view) or as a param-cased directory (packages/app-view). I'm unsure of which should be the default - let's have a think about this. We may need a flag here to determine this.

Good point, that needs a bit more work.

Done, not renaming directories as discussed.

packages/modular-scripts/src/rename.ts

cangarugula · 2021-12-14T17:06:10Z

packages/modular-scripts/src/rename.ts

+    )
+  ).filter((pkgJsonInfo) =>
+    dependencyTypes.some(
+      (depField) => pkgJsonInfo.json[depField]?.[oldPackageName],


What's the use case here? If a package is using another package in the same monorepo, they shouldn't need to specify it as a dependency in their package.json. The one case I can think of is if they want to specify an older version of the dependency, in which case, you wouldn't want to change it to the new package name. Maybe I'm missing a use case here.

If a package is using another package in the same monorepo, they shouldn't need to specify it as a dependency in their package.json

Don't they need to specify the dependency in their package.json to get the other package symlinked? I'm fairly new to yarn workspaces but that's what the documentation example says: https://classic.yarnpkg.com/lang/en/docs/workspaces/#toc-how-to-use-it (note that it's not necessary to build package-a, if you don't need it outside the monorepo you just point main or module or exports to src)

There's always the case that package-b just imports from package-a's source using a relative path (eg import libA from "../package-a/src/index.js"), but then what's the advantage of having workspaces / a monorepo (ie decoupling packages etc)?

They don't need to specify the dependency to use the package immediately. Because we set "workspaces": ["packages/**] and we call "yarn" after we create a new package with /packages, yarn creates a symlink to the local package in your node_modules, which can be imported/required using its package name.

But they would need to add it explicitly is if they wanted to externalize the dependency from the build, which case would be to published version that can be installed from the registry. But I can also see the value of doing it anyways just in case they do have the dependency there.

Done, removed renaming of dependencies in package.jsons (I was used to rushjs, where you have to specify internal dependencies. Good to know!)

cangarugula · 2021-12-14T17:08:40Z

.gitignore

 packages/nested
+packages/sample-library-package
+packages/sample-depending-package
+packages/sample-library-renamed-package


let's add a scoped and nested example - which will be the majority of apps for our teams

Now that we don't actually rename directories, do we still need this? Wouldn't like to pollute the repo too much.

I don't think we need the scoped examples anymore. And we won't need packages/sample-library-renamed-package because we aren't changing the directory name anymore.

You're right, done.

cangarugula · 2021-12-16T09:22:09Z

packages/modular-scripts/src/rename.ts

+      });
+      await project.save();
+    }),
+  );


If you change the package name in the package.json, you'll need to do a execa.sync('yarnpkg') to update the the name of the symlink in node_modules

cangarugula · 2022-01-05T10:45:45Z

.changeset/nasty-mayflies-sin.md

+"modular-scripts": minor
+---
+
+Add "modular rename" command


Let's add a tiny summary of what rename will do so that the release notes generated will have a bit more info on this

Done, but still really tiny (a more detailed description will be added in the next release notes)

cangarugula · 2022-01-05T10:49:56Z

.gitignore

 packages/nested
+packages/sample-library-package
+packages/sample-depending-package
+packages/sample-library-renamed-package


I don't think we need the scoped examples anymore. And we won't need packages/sample-library-renamed-package because we aren't changing the directory name anymore.

packages/modular-scripts/src/__tests__/__fixtures__/packages/sample-depending-package/index.tsx

cangarugula · 2022-01-05T10:56:21Z

packages/modular-scripts/src/program.ts

  });

+program
+  .command('rename <old-package-name> <new-package-name>')


We use camel case for command variables some places and pascal case in others. Since our options are camel cased, maybe we could stick with that format?

cangarugula · 2022-01-05T11:01:54Z

packages/modular-scripts/src/rename.ts

+
+interface PackageJson extends Partial<DependencyObject> {
+  name: string;
+}


You should also be able to get those dependencty types from import { CoreProperties } from '@schemastore/package'

Good catch, this was handy before, where I iterated on the depTypes const array and constructed the types from it, but it's not needed now. Done.

cangarugula · 2022-01-05T11:03:03Z

packages/modular-scripts/src/rename.ts

+  }
+
+  try {
+    logger.debug(`Checking for existence of ${oldPackageName} in workspace.`);


We should check for the existence and collisions before we ask people to stash their changes.

cangarugula · 2022-01-05T11:07:16Z

packages/modular-scripts/src/rename.ts

+    );
+
+    logger.log(`Refreshing packages`);
+    execa.sync('yarnpkg');


Let's silence the install and specific the cwd as modular root in case they're not in the root. execa.sync('yarnpkg', ['--silent'], { cwd: modularRoot })

Stub rename command

564d800

Cristiano Belloni added 4 commits December 8, 2021 17:11

Search for depending packages

ff9fcbd

Rename dependencies in depending packages

7d7939f

re-write imports with ts-morph

999ab69

cleanup

a00bbec

cristiano-belloni requested review from LukeSheard and cangarugula December 9, 2021 17:07

Cristiano Belloni and others added 6 commits December 10, 2021 09:52

Fix await to wait for codemod promises

83dea44

Logging

88e51fe

Add tests

55ee463

Add docs

1e400bd

Correct docs

8b26078

Create nasty-mayflies-sin.md

7b12712

cristiano-belloni marked this pull request as ready for review December 13, 2021 16:26

cristiano-belloni changed the title ~~Rename command~~ modular rename command Dec 13, 2021

Cristiano Belloni added 2 commits December 13, 2021 18:08

Add js, jsx files

e771045

Merge branch 'feature/add-rename' of https://github.com/jpmorganchase…

235cc55

…/modular into feature/add-rename

cangarugula reviewed Dec 14, 2021

View reviewed changes

cangarugula reviewed Dec 16, 2021

View reviewed changes

Cristiano Belloni added 7 commits December 17, 2021 15:32

Merge remote-tracking branch 'origin/main' into feature/add-rename

64f837e

Refactor to not rename directories

6588cdb

Remove changes in other package.jsons

ae67bef

Fix test

169e118

Don't use fixture

6a4eb40

Make rename atomic

e60cce7

Update docs and test label

8cbf3e6

cangarugula reviewed Jan 5, 2022

View reviewed changes

Update changeset

fbb0233

cangarugula reviewed Jan 5, 2022

View reviewed changes

Cristiano Belloni added 5 commits January 5, 2022 13:16

Use camelCase in options

67fa34e

use CoreProperties

ce2ab5f

Check for existence / collision before asking user to stash

c70f6ab

Silence yarnpkg output

e3f305d

ignore renamed directory

4c949e9

cangarugula approved these changes Jan 5, 2022

View reviewed changes

Merge branch 'main' into feature/add-rename

40ad365

cristiano-belloni merged commit e180686 into main Jan 5, 2022

cristiano-belloni deleted the feature/add-rename branch January 5, 2022 15:13

github-actions bot mentioned this pull request Jan 5, 2022

Version Packages #1261

Merged

github-actions bot mentioned this pull request Feb 3, 2022

Version Packages joshwooding/modular#1

Closed

github-actions bot mentioned this pull request Feb 24, 2022

Version Packages benpryke/modular#1

Open

This was referenced Mar 16, 2022

Version Packages (next) #1484

Closed

Version Packages (next) #1485

Closed

Conversation

cristiano-belloni commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

coveralls commented Dec 8, 2021 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristiano-belloni Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cangarugula Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Reviewers

cristiano-belloni commented Dec 8, 2021 •

edited

Loading

changeset-bot bot commented Dec 8, 2021 •

edited

Loading

coveralls commented Dec 8, 2021 •

edited

Loading

cristiano-belloni Dec 14, 2021 •

edited

Loading

cangarugula Jan 5, 2022 •

edited

Loading

cristiano-belloni Jan 5, 2022 •

edited

Loading