feat: initial makeGasket with actions #720
Conversation
|
| Name | Status | Previous | Current |
|---|---|---|---|
@es-joy/jsdoccomment |
- | 0.42.0 | |
@gasket/core |
- | file:packages/gasket-core | |
are-docs-informative |
- | 0.0.2 | |
comment-parser |
- | 1.4.1 | |
eslint-config-godaddy |
7.0.2 | 7.1.0 | |
eslint-plugin-jsdoc |
- | 48.2.3 | |
is-builtin-module |
3.2.0 | 3.2.1 | |
jsdoc-type-pratt-parser |
- | 4.0.0 | |
undici-types |
- | 5.26.5 |
| * @returns config | ||
| */ | ||
| export function applyConfigOverrides(config: GasketConfig, { env, commandId, root, localFile }: { | ||
| export function applyConfigOverrides(config: GasketConfigDefinition, { env, commandId }: { |
There was a problem hiding this comment.
commandId still relevant?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
commandId comes from where?
| "description": "Entry point to setting up Gasket instances", | ||
| "main": "lib/index.js", | ||
| "types": "lib/index.d.ts", | ||
| "type": "module", |
There was a problem hiding this comment.
One small step for ESM ✊
| * @returns {string} env | ||
| */ | ||
| function getEnvironment( | ||
| // flags, commandId, warn |
There was a problem hiding this comment.
Commented code not sure if it's intentional
There was a problem hiding this comment.
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]}'` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
|
||
| /** @type {import('@gasket/engine').Plugin} */ |
There was a problem hiding this comment.
The tsconfig.json is currently configured to skip typechecking anything in the test directory.
There was a problem hiding this comment.
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?
Summary
Add
@gasket/corepackage to make available the new makeGasket approach with which allows "Gaskets" to imported to app code and functionality executed byactions.Changelog
@gasket/core
actionslifeycycleconfigureto be syncrounous@gasket/engine
@gasket/plugin-nextjs
getNextConfigactiongetWebpackConfigaction from webpack plugin@gasket/plugin-webpack
getWebpackConfigaction, which replaces previousinitWebpackfunctionTest Plan
Basics were tested in a Next14 app using App Router