PDE-4678 fix(cli): `zapier build` fails with npm workspaces #738

eliangcs · 2024-01-30T13:00:30Z

Fixes #648.

Some developers may choose to structure their project using npm workpaces or yarn workspaces. An example project structure might look like:

(project root)
├─ package.json
└── packages/
    ├─ app-1/
    │  ├─ index.js
    │  └─ package.json
    └─ app-2/
        ├─ index.js
        └─ package.json

If you run npm install in the above example, app-1 and app-2 will share their dependencies in the project root's node_modules. Only the dependencies with version discrepancies will be placed into app-1/node_modules or app-2/node_modules.

The current implementation of zapier build doesn't support workspaces. It always assume it can always find zapier-platform-core in the working directory's node_modules.

This PR fixes that by finding zapier-platform-core in the working directory and all the parent directories.

eliangcs · 2024-01-30T13:06:16Z

@rnegron this is still work-in-progress as I'd like to add some test cases. But you might want to take a look first, because I also modified copyDir() as you did in #737.

rnegron · 2024-01-30T16:02:06Z

@eliangcs

But you might want to take a look first, because I also modified copyDir() as you did in #737.

Thanks for letting me know! Let's use this PR for those initial async/await changes, since I will likely modify it further to add some fs.lstatSync(...).isSymbolicLink() related logic!

eliangcs · 2024-01-31T12:43:51Z

@rnegron this is ready for review. Thanks!

rnegron · 2024-01-31T15:12:40Z

packages/cli/src/tests/utils/build.js

@@ -309,3 +311,189 @@ describe('build (runs slowly)', function () {
    should.equal(buildExists, true);
  });
 });
+
+describe('build in workspaces', function () {


Great and easy to follow tests! 👍🏻

rnegron · 2024-01-31T15:13:43Z

packages/cli/src/tests/utils/files.js

@@ -38,27 +37,137 @@ describe('files', () => {
      .catch(done);
  });

-  // TODO: this is broken in travis - works locally though


rnegron · 2024-01-31T15:14:58Z

packages/cli/src/utils/build.js


-  startSpinner('Building app definition.json');


I'm guessing this spinner was removed intentionally?

It was a mistake. Added it back in 52fba09. Thanks!

rnegron · 2024-01-31T17:31:05Z

packages/cli/src/utils/files.js

+  Returns a promise that copies a directory recursively.
+


👍🏻 Always appreciate these descriptive comments

This reverts commit 4ff1ba9.

[PDE-4679] fix(cli): better handling of broken symlinks while copying files to temp directory during build process

eliangcs · 2024-02-01T12:05:04Z

@rnegron I merged your PR #737 and added some code to exclude workspace symlinks, such as node_modules/app-1 and node_modules/app-2, from the build.zip. Can you re-review? Thanks a lot!

rnegron · 2024-02-01T19:48:01Z

packages/cli/src/utils/build.js

+              fse.readlinkSync(src)
+            );
+            for (const workspace of workspaces) {
+              if (minimatch(realPath, workspace)) {


@eliangcs Could we add a comment here explaining what this minimatch line is doing? I believe it's true if the src symlink resolves (points to) to a workspace path, in which case we want to filter out (return false) this particular src. So that we don't copy over any symlinks that point to workspace folders.

You interpretation is correct. Since the workspaces array in the root package.json can contain glob patterns, like packages/*, I use minimatch to help us do glob pattern matching. For example, minimatch('/path/to/packages/app-1', '/path/to/packages/*') returns true.

I'll add a comment in a separate PR. Thanks!

fix(cli): zapier build fails with npm workspaces

ec5aeea

eliangcs changed the title ~~fix(cli): zapier build fails with npm workspaces~~ PDE-4678 fix(cli): zapier build fails with npm workspaces Jan 30, 2024

eliangcs added 2 commits January 31, 2024 15:50

Add tests for zapier build in workspaces

05ac7b7

Fix box drawing tree

1397398

eliangcs marked this pull request as ready for review January 31, 2024 08:11

eliangcs requested a review from rnegron as a code owner January 31, 2024 08:11

Make zapier validate support workspaces too

6cfae02

rnegron previously approved these changes Jan 31, 2024

View reviewed changes

rnegron and others added 5 commits January 31, 2024 14:45

check gitignore before copying to tmpDir

4ff1ba9

Revert "check gitignore before copying to tmpDir"

dff25dd

This reverts commit 4ff1ba9.

rebase and use warning logic in copyDir

c1a68fe

simplify logic to: if stat fails, check if symlink and notify if so

7968c49

Add startSpinner back

52fba09

eliangcs dismissed rnegron’s stale review via 52fba09 February 1, 2024 06:06

eliangcs added 5 commits February 1, 2024 14:08

Use spaces instead tabs

a826667

Merge branch 'PDE-4678-build-in-workspaces' into PDE-4679

012dcdf

Merge pull request #737 from zapier/PDE-4679

f9687d7

[PDE-4679] fix(cli): better handling of broken symlinks while copying files to temp directory during build process

Exclude workspace symlinks from build.zip

dacbf6d

Add test code

d5f1c45

rnegron approved these changes Feb 1, 2024

View reviewed changes

eliangcs merged commit 3ce7c83 into main Feb 2, 2024
13 checks passed

eliangcs deleted the PDE-4678-build-in-workspaces branch February 2, 2024 05:19

eliangcs mentioned this pull request Feb 2, 2024

PDE-4678 fix(cli): add comments and fix a path issue in utils/build.js #742

Merged

This was referenced Feb 2, 2024

[PDE-3790] chore: release 15.5.2 #744

Merged

PDE-4678 fix(cli): Use findCorePackageDir() utility method when importing zapier-platform-core #746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDE-4678 fix(cli): `zapier build` fails with npm workspaces #738

PDE-4678 fix(cli): `zapier build` fails with npm workspaces #738

eliangcs commented Jan 30, 2024 •

edited

Loading

eliangcs commented Jan 30, 2024

rnegron commented Jan 30, 2024

eliangcs commented Jan 31, 2024

rnegron Jan 31, 2024

rnegron Jan 31, 2024

rnegron Jan 31, 2024

eliangcs Feb 1, 2024

rnegron Jan 31, 2024

eliangcs commented Feb 1, 2024

rnegron Feb 1, 2024

eliangcs Feb 2, 2024 •

edited

Loading

PDE-4678 fix(cli): zapier build fails with npm workspaces #738

PDE-4678 fix(cli): zapier build fails with npm workspaces #738

Conversation

eliangcs commented Jan 30, 2024 • edited Loading

eliangcs commented Jan 30, 2024

rnegron commented Jan 30, 2024

eliangcs commented Jan 31, 2024

rnegron Jan 31, 2024

Choose a reason for hiding this comment

rnegron Jan 31, 2024

Choose a reason for hiding this comment

rnegron Jan 31, 2024

Choose a reason for hiding this comment

eliangcs Feb 1, 2024

Choose a reason for hiding this comment

rnegron Jan 31, 2024

Choose a reason for hiding this comment

eliangcs commented Feb 1, 2024

rnegron Feb 1, 2024

Choose a reason for hiding this comment

eliangcs Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

PDE-4678 fix(cli): `zapier build` fails with npm workspaces #738

PDE-4678 fix(cli): `zapier build` fails with npm workspaces #738

eliangcs commented Jan 30, 2024 •

edited

Loading

eliangcs Feb 2, 2024 •

edited

Loading