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

support A3 projects with npm workspaces packages #4284

Merged
merged 18 commits into from
Sep 12, 2023

Conversation

haroun
Copy link
Contributor

@haroun haroun commented Sep 1, 2023

Summary

Add support for direct workspace dependencies

What are the specific steps to test this change?

You run the test test/workspaces-project.js or

  1. Start from a3-boilerplate
  2. Run npm init -w workspace-a -y to create workspace-a
  3. Install @apostrophecms-pro/scheduled-publishing in workspace-a with npm install -w workspace-a --save @apostrophecms-pro/scheduled-publishing
  4. In the a3-boilerplate project, enable the module @apostrophecms-pro/scheduled-publishing. It should not be present in the a3-boilerplate package.json. It should pass.
  5. Switch to apostrophe main branch, the same thing should fail with The configured bundle @apostrophecms-pro/scheduled-publishing was not found in npm

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@haroun haroun self-assigned this Sep 1, 2023
@linear
Copy link

linear bot commented Sep 1, 2023

PRO-4733 Add support for `npm workspaces` in `moog-require`

They are now using npm workspaces, and when a project uses workspaces, Apostrophe is unable to detect sub-packages and that's an npm feature.

Decision is to respect npm workspaces. The code should be written to understand what workspaces are active in the project and honor modules in those workspaces just like it honors the packages in package.json itself.

Acceptance criteria

A package in a workspace can be loaded as an Apostrophe module.

Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

General strategy makes sense, see notes. Thanks

@@ -7,6 +7,30 @@ const importFresh = require('import-fresh');
const resolveFrom = require('resolve-from');
const regExpQuote = require('regexp-quote');

const getDependencies = ({
Copy link
Member

Choose a reason for hiding this comment

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

Use function and place these at the end, don't front-load the file with implementation detail code that makes it harder to read the overall intent.

return [];
}

const workspacePackages = glob.sync(`{${workspaces.join(',')}}/package.json`, { follow: true });
Copy link
Member

Choose a reason for hiding this comment

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

Do not make assumptions about the current working directory. Note how the code that calls getWorkspacesDependencies figures out what folders to look at on each iteration and pass folder into getWorkspacesDependencies so you can concatenate it with the workspace name.

@haroun haroun requested a review from boutell September 7, 2023 08:20
@haroun haroun marked this pull request as ready for review September 7, 2023 13:05
CHANGELOG.md Outdated

### Adds

* Add support for npm workspaces dependencies. A workspace dependencies can now be used as an Apostrophe module even if it is not a direct dependency of the Apostrophe project. Only direct workspaces dependencies of the Apostrophe project are supported. e.g. I have an Apostrophe project called `website`. `website` is set with 2 workspaces, `workspace-a` & `workspace-b`. `workspace-a` `package.json` contains a module named `blog` as a dependency. `website` can reference `blog` as enabled in the `modules` configuration.
Copy link
Member

Choose a reason for hiding this comment

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

A workspaces dependencies -> A workspaces dependency

"For instance, I have..."

"With 2 workspaces" -> "with two npm workspaces"

Link "npm workspaces" to relevant npm documentation on the first mention

(And sync up with Bob for docs)

return [];
}

const pattern = workspaces.length === 1 ? workspaces[0] : `{${workspaces.join(',')}}`;
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure the join call is always sufficient here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment, glob expects at least two entries when using curly braces. Silly bug!


const pattern = workspaces.length === 1 ? workspaces[0] : `{${workspaces.join(',')}}`;
const packagePath = path.resolve(folder, pattern, 'package.json');
const workspacePackages = glob.sync(packagePath, { follow: true });
Copy link
Member

Choose a reason for hiding this comment

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

niiice

Copy link
Member

Choose a reason for hiding this comment

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

Very slick use of advanced glob features

@haroun haroun requested a review from boutell September 8, 2023 08:55
* main:
  fix code overthinking
  Response to first comments
  Add note to CHANGELOG
  expands per area configuration
boutell
boutell previously approved these changes Sep 8, 2023
@@ -0,0 +1,40 @@
const createLogger = () => {
Copy link
Member

Choose a reason for hiding this comment

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

good technique.

* main:
  hotfix for logging non-Error exceptions from api routes
  typo
  works
  update confirm message
  replace --force with --confirm + remove prompts for hosted customer users
  add --force to skip prompt
  remove stray log
  use prompts and update changelog
  figure should reset the wrapper
  handle paragraph without texts
  add lint-fix and remove-empty-paragraph task to rich-text
@haroun haroun requested a review from boutell September 12, 2023 14:08
@haroun haroun merged commit f645fe6 into main Sep 12, 2023
6 checks passed
@haroun haroun deleted the pro-4733-npm-workspace-support branch September 12, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants