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

Base project discovery around package.json first #70

Merged
merged 3 commits into from
May 22, 2023

Conversation

g-elwell
Copy link
Member

@g-elwell g-elwell commented Mar 30, 2023

This PR is branched from tests added in #67 so should be reviewed after that is merged

Description

Within the build-tools we have an understanding that a "project" should contain both a package.json and src/entrypoints but this isn't applied consistently across different modes of operation.

This makes it more complicated to grasp how projects are gathered and ensure that invalid attempts to run the build command present the user with informative error messages, since different logic and error handling are applied in each scenario.

This PR alters the discovery process, to first gather all package.json that we can find (within the root or targetDirs depending on mode), then filter these based on whether src/entrypoints is found relative to each package. This allows us to apply consistent logic across different modes and reduce the amount of error cases we need to handle.

Conceptually, it makes sense for the package.json to be the main indicator for a project as this is relied on to be the source of truth for the project name and version.

The current method used for gathering projects is:

  1. Apply context/flags (to determine which mode to run in)
  2. Gather project paths
    1. Single mode - returns current path
    2. Site mode - returns paths to all sub-directories in the global targetDirs which contain src/entrypoints
    3. List mode - returns paths to all listed sub-directories if they exist inside one of the targetDirs
  3. Filter paths based on package.json being present

The updated method looks like this:

  1. Apply context/flags (to determine which mode to run in)
  2. Gather project package objects (package.json + path info)
    1. Single mode - returns package object from current path if found
    2. Site mode - returns package objects from all sub-directories in the global targetDirs
    3. List mode - as "site mode" but filters the results to include only projects requested by the user
  3. Filter packages based on src/entrypoints being present

Side effects

This change would also mean that "list" mode uses the package project name, rather than the directory name, which is preferred IMO as this could cause confusion previously when the directory name and package name were different.

It also handles the error case outlined in #69

Change log

  • get-package.js - getPackage no longer optionally throws an error, which wasn't being utilised previously
  • projectpaths.js - findAllProjectPaths updated to rely on package.json instead of src/entrypoints being present, other functions removed as no longer required, validateProject added which confirms src/entrypoints exists relative to package.json
  • build.js - refactoring project discovery, improved error handling
  • Unit/Integration tests updated to reflect adjusted behaviours and error handling

@g-elwell g-elwell marked this pull request as ready for review March 30, 2023 19:08
@ampersarnie ampersarnie added the enhancement New feature or request label Apr 10, 2023
@ampersarnie ampersarnie added this to the v1.3 milestone Apr 10, 2023
@ampersarnie ampersarnie linked an issue Apr 10, 2023 that may be closed by this pull request
@ampersarnie ampersarnie changed the base branch from main to release/1.3 April 17, 2023 15:02
Copy link
Member

@ampersarnie ampersarnie left a comment

Choose a reason for hiding this comment

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

I've had a look at this and done some testing and everything mostly works as expected. My only concern is around this:

This change would also mean that "list" mode uses the package project name, rather than the directory name, which is preferred IMO as this could cause confusion previously when the directory name and package name were different.

This particular point makes this PR breaking change as previous workflows would no longer work when targetting specific projects. Or rather it should, as it led me to find an issue.

In src/utils/get-package.js the name property is split where there is a namespace to get the plugin/theme name. While previously this was only used for display purposes, this name property is now used for targetting the projects. So what we have is if we add a namespace to the example-site/plugins/test-plugin so it's @namespace/test-plugin. We get the following;

Failure

➜  example-site git:(feat/project-discovery) ✗ node ../src/cli.js build --once @namespace/test-plugin
Compiling list of projects in development mode.
Error: No projects found

Success

➜  example-site git:(feat/project-discovery) ✗ node ../src/cli.js build --once test-plugin 
Compiling list of projects in development mode.
Processing the following projects:
 * test-plugin [plugins/test-plugin/package.json]
...

If the intention is to use the package name then we should use that in full to avoid 1. confusion over what is expected for the project values, and 2. any potential clashes where we may have packages with the same name.

However, this does give me pause for concern about having potentiallylengthy commands to target specific projects, so in this case I'm leaning towards keeping the directory name as what is defined for the project parameters.

build-tools build --once @namespace/test-plugin,@namespace/some-other-plugin,@namespace/test-theme

vs.

build-tools build --once test-plugin,some-other-plugin,test-theme

@g-elwell
Copy link
Member Author

@ampersarnie good point, thanks.

It should be a minor change to update the list mode discovery to ignore subdirectories that aren't part of the user-provided input.

That way we can get the benefits this PR brings whilst avoiding any breaking changes. I'll try it out and keep you posted.

@g-elwell
Copy link
Member Author

@ampersarnie the latest commit adjusts list mode to utilise the directory name during project discovery to retain the original behaviour.

This is achieved by passing an optional extra parameter to findAllProjectPaths which, if provided, will limit the paths returned to only those requested by the user.

This doesn't alter the behaviour outlined in the original PR description, other than removing the breaking change of using the package name instead of the project directory name.

@ampersarnie ampersarnie self-requested a review May 22, 2023 12:47
@ampersarnie ampersarnie merged commit 63f6029 into release/1.3 May 22, 2023
@ampersarnie ampersarnie mentioned this pull request Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Report when there are no projects found.
2 participants