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

chore(atomic): split create-atomic and its template altogether #714

Merged
merged 34 commits into from
Mar 18, 2022

Conversation

louis-bompart
Copy link
Collaborator

@louis-bompart louis-bompart commented Mar 8, 2022

Motivation

In npm/cli#4518, I found out that 'sub-packages' with npm workspaces are not supported.

Decision

From there two solutions:

  1. Do not use a subpackage for the template
  2. Remove the subpackage occurrence

Pro for 1:

  • Hierarchy stay the same
  • The hierarchy represents better what's shipped to the end-user.

Con for 1:

  • It would require dedicated scripts to integrate with our monorepo architecture.

Pro for 2:

  • Smaller packages, with smaller jobs, making them more understandable.
  • It works nicely with our monorepo architecture

Con for 2:

  • Opposites of pro for 1.

Conclusion

I chose to go with option 2 because it was really simpler to put in place, and I think long-term it will help to distinguish between the 'leg-work' done by the generator, and what's actually generated.

It's important to note that the 'private' fields of the packages did not change, meaning that the template is still unpublished and that it's still bundled with @coveo/create-atomic, so no changes for the end-user.

Testing

No logic changes, the bundling process will be tested by the CI.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

Thanks for your contribution @louis-bompart !
When your pull-request is ready to be merged, check the box below to merge it

  • Merge! :shipit:

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

@louis-bompart louis-bompart force-pushed the CDX-868 branch 2 times, most recently from cbd76e0 to 528ea10 Compare March 8, 2022 21:22
Base automatically changed from chore/CDX-866 to master March 10, 2022 03:36
Copy link
Contributor

@ThibodeauJF ThibodeauJF left a comment

Choose a reason for hiding this comment

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

Funny how snyk got us there, template shouldn't even be a sub-package 😄

@louis-bompart
Copy link
Collaborator Author

On hold, waiting for npm/map-workspaces#32 to be merged & released. Then for npm/cli to update its dep and release.

@louis-bompart louis-bompart marked this pull request as draft March 10, 2022 16:30
@louis-bompart
Copy link
Collaborator Author

Funny how snyk got us there, template shouldn't even be a sub-package 😄

Yeah, it is true that from a technical standpoint this is unnecessary, one could even say silly, I totally agree.

However, the boons it brings outweigh the cost by far imo; thanks to this pattern we are able to ensure the template we ship to our customer is using 'just the right' versions: not the past one, nor the future one.

Dependencies bundled/related to the template are also kept to date, and we also ensure that there are no malicious dependencies shenanigans going on.

The sub-packaging also allows the template to be 'just right' on publication thanks to topological releases.

@louis-bompart louis-bompart marked this pull request as ready for review March 10, 2022 19:03
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.

None yet

3 participants