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

feat: initial makeGasket with actions #720

Merged
merged 23 commits into from
Apr 11, 2024
Merged

feat: initial makeGasket with actions #720

merged 23 commits into from
Apr 11, 2024

Conversation

agerard-godaddy
Copy link
Contributor

@agerard-godaddy agerard-godaddy commented Apr 3, 2024

Summary

Add @gasket/core package to make available the new makeGasket approach with which allows "Gaskets" to imported to app code and functionality executed by actions.

Changelog

@gasket/core

  • Initial release
  • Add actions lifeycycle
  • Change configure to be syncrounous

@gasket/engine

  • Plugin engine only takes plugins as argument
  • No config setup: Config now managed completed by core package
  • No plugin resolving: Plugins must be loaded and passed to engine
  • Types were changed to match new signature

@gasket/plugin-nextjs

  • Added getNextConfig action
  • Uses getWebpackConfig action from webpack plugin

@gasket/plugin-webpack

  • Added getWebpackConfig action, which replaces previous initWebpack function

Test Plan

Basics were tested in a Next14 app using App Router

Copy link

github-actions bot commented Apr 5, 2024

package-lock.json changes

Click to toggle table visibility
Name Status Previous Current
@es-joy/jsdoccomment ADDED - 0.42.0
@gasket/core ADDED - file:packages/gasket-core
are-docs-informative ADDED - 0.0.2
comment-parser ADDED - 1.4.1
eslint-config-godaddy UPDATED 7.0.2 7.1.0
eslint-plugin-jsdoc ADDED - 48.2.3
is-builtin-module UPDATED 3.2.0 3.2.1
jsdoc-type-pratt-parser ADDED - 4.0.0
undici-types ADDED - 5.26.5

@agerard-godaddy agerard-godaddy marked this pull request as ready for review April 9, 2024 17:52
@agerard-godaddy agerard-godaddy requested review from kinetifex and a team as code owners April 9, 2024 17:52
* @returns config
*/
export function applyConfigOverrides(config: GasketConfig, { env, commandId, root, localFile }: {
export function applyConfigOverrides(config: GasketConfigDefinition, { env, commandId }: {
Copy link
Contributor

Choose a reason for hiding this comment

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

commandId still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dunno yet - it might still be so leaving until we make a final decision


export function loadGasketConfigFile(
export function loadGasketConfigDefinition(
root: string,
env: string,
commandId: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

commandId comes from where?

"description": "Entry point to setting up Gasket instances",
"main": "lib/index.js",
"types": "lib/index.d.ts",
"type": "module",
Copy link
Contributor

Choose a reason for hiding this comment

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

One small step for ESM ✊

* @returns {string} env
*/
function getEnvironment(
// flags, commandId, warn
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented code not sure if it's intentional

Copy link
Contributor Author

@agerard-godaddy agerard-godaddy Apr 10, 2024

Choose a reason for hiding this comment

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

I left these commented out as there might still be a scenario where commands and flags could come into play. I'll add a todo so we remember to clean them up and/or implement - I don't want to loose track of them for now

if (actionPluginMap[actionName]) {
// eslint-disable-next-line no-console
console.error(
`Action '${actionName}' from '${plugin.name}' was registered by '${actionPluginMap[actionName]}'`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any concern of naming collisions in the future between plugins that have actions with the same name?

It looks like this line takes care of the problem with actions having the same name but I'm wondering in the future when more actions are created if we will run into this issue more often and then we have to make a name change and a publish to fix it.

We could potentially look into aliasing action names? gasket.actions.<plugin-short-name>.<action-name>. It's uglier but could help with collisions.

This might not be a problem but just wanted to bring up this thought to see what the team thinks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah you're saying like namespacing actions.....

I think that could be potentially valuable, at the same time I don't think the collisions will be a huge concern. Actions should be doing something rather specific in the scope and the names should be probably end up being rather unique. Interested to see what Andrew thinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I think it'll be important that the action names be unique. Aliases could work possibly in application code, but we will not be able to indicate them properly with Types for intellisense and type checks. Also, some plugins will actually execute the actions of other plugins they depend on, in which case they will not know about any aliasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If anything, we may consider adding a 'strict' option or something that hard fails if duplicate action names are registered. That or make it the default behavior.

@agerard-godaddy agerard-godaddy added the version 7 Change targeted for version 7 label Apr 10, 2024
packages/gasket-core/README.md Outdated Show resolved Hide resolved
packages/gasket-core/lib/index.js Outdated Show resolved Hide resolved
packages/gasket-core/lib/index.js Show resolved Hide resolved
packages/gasket-core/lib/index.js Show resolved Hide resolved
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});

/** @type {import('@gasket/engine').Plugin} */
Copy link
Contributor

Choose a reason for hiding this comment

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

The tsconfig.json is currently configured to skip typechecking anything in the test directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes good to know - I don't think it hurts to have these here for reference at least. Perhaps we should enable type checks for some tests?

packages/gasket-core/package.json Outdated Show resolved Hide resolved
packages/gasket-engine/lib/engine.js Show resolved Hide resolved
packages/gasket-engine/package.json Show resolved Hide resolved
@agerard-godaddy agerard-godaddy merged commit 3bb0dc9 into v7 Apr 11, 2024
4 checks passed
@agerard-godaddy agerard-godaddy deleted the gasket-actions branch April 11, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version 7 Change targeted for version 7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants