[refactor] Better integrate new internal plugins #108
Conversation
…on + uniformisation
yoannmoinet
changed the title
yoannmoinet
force-pushed
the
yoann/new-internal-plugins
branch
2 times, most recently
from
9757247
to
82ed7f6
Compare
yoannmoinet
changed the title
| bundler: { | ||
| name: `[${yellow('Discouraged')}] Bundler Specific Plugin`, | ||
| descriptions: [ | ||
| 'Create a plugin that will be customer facing.', |
There was a problem hiding this comment.
Super nit: it feels weird to me to have different wording for the same thing ('Create a customer facing plugin.' / 'Create a plugin that will be customer facing.')
There was a problem hiding this comment.
Agreed 100%, I'll update.
| @@ -172,7 +244,7 @@ const verifyCodeowners = (plugins: Workspace[]) => { | |||
|
|
|||
| export const updateFiles = async (plugins: Workspace[]) => { | |||
| const errors: string[] = []; | |||
| await updateFactory(plugins); | |||
| errors.push(...(await updateFactory(plugins))); | |||
There was a problem hiding this comment.
Is there some reason that prevents doing this as a one liner?
const errors: string[] = await updateFactory(plugins);There was a problem hiding this comment.
Absolutely no reason at all, apart from some refactoring residue.
I'll update.
| const paramsString = pluginExports[getFunction] | ||
| .toString() | ||
| // Find the list of arguments. | ||
| .match(/^\(([^)]+)\)/)[1] |
There was a problem hiding this comment.
Is pluginExports[getFunction]always well formatted? If the argument list contains destructuring patterns or extra spacing / new lines it could mess up the regex matching and the subsequent split.
There was a problem hiding this comment.
Agreed this is flimsy and hacky.
It's basically a poor-man's dependency injection, doing best-effort.
This is "mostly" safe for a few reasons:
- the code it parses is coming from a wizard that templates this part.
- the code is expected to be linted/prettiered, but true that someone could disable this specific line.
- the result is verified against a list of available params, so if I don't find one, it raises the error.
- this would only happen on the DD dev side when creating/updating an internal plugin, and would never be able to reach production as it would most likely fail either in the tests, the linting or the typechecking.
I will improve the RegEx to cover more ground than what it does right now.
And keep an eye out in case we hit edge cases in the future.
What and why?
Now that we consider internal plugins almost the same as external plugins,
we need to better integrate how we create and verify them.
How?
yarn cli create-plugin.yarn cli integrity.Note
I will remove
Check integrity of our filesfrom the expected checks once this PR is merged.