refactor(plugins): switched from `require` to `await import()` when loading plugins #2558

travi · 2022-09-12T04:43:58Z

to isolate the conversion ahead of #2543

…oading plugins to isolate the conversion ahead of #2543

travi · 2022-09-12T04:53:33Z

this obviously still needs clean up, but hoping to get some feedback. i'm not 100% i have my head around all that needs to change here, but assuming we change from require to await import(), the majority of the work should be handling the additional promises that are introduced. i think i have most of that handled, but may have missed some.

however, the part that i did not anticipate is that the result does not appear to satisfy _.isPlainObject() the same way that loading with require did. it does not appear to be an issue with the added default property either.

logging one of the example plugins from the tests shows the following, which i would expect to both satisfy _.isPlainObject (at least because of the null prototype) and satisfy the expected shape if it were to make it far enough through the logic

[Module: null prototype] {
  analyzeCommits: [AsyncFunction: analyzeCommits],
  default: { analyzeCommits: [AsyncFunction: analyzeCommits] }
}

but it appears that the loaded module has some detail that i havent yet figured out that prevents it from satisfying _.isPlainObject()

travi · 2022-09-12T04:58:43Z

this also got me thinking about how this situation of loading plugins potentially gets more complicated as we progress further. currently, only cjs plugins are expected. once we move to esm, we will want to support loading plugins that are also esm. loading with await import() likely normalizes that situation for us, but we will want to confirm it once we are past this part.

this partially comes to mind because i did consider if it could be worth considering using module.createRequire to avoid all of the complexity, but that would prevent supporting esm plugins. i dont think that even makes sense as an intermediate step since we will want to convert official plugins to esm as soon as converting core to esm unblocks converting the plugins.

gr2m

changes look good to me thus far

gr2m · 2022-09-12T21:14:37Z

lib/plugins/index.js

+        console.log({plugins: options.plugins})
+        console.dir(plugin, {depth: null})
+        console.log(isPlainObject(plugin))


just a reminder to remove the debug logs when the PR is ready :)

yep 👍🏼

gr2m · 2022-09-12T21:15:33Z

lib/plugins/index.js

-          const [name, config] = parseConfig(plugin);
-          plugin = isString(name) ? loadPlugin(context, name, pluginsPath) : name;
+    ? await castArray(options.plugins).reduce(async (plugins, plugin) => {
+      const plugins2 = await plugins;


I wonder if there is a better name for plugins2? Maybe normalizedPlugins or awaitedPlugins?

yep, this is definitely part of the clean up that is necessary once this is working :)

and fixed paren grouping

… into async-import

travi · 2022-09-16T22:18:31Z

i was able to resolve the previous issues by fixing the paren wrapping that i broke (this was the main problem) and using the .default property of the imported module (which i had tried previously, but wasnt working fully because of the former problem

travi · 2022-09-16T22:22:05Z

however, xo now reports a lint issue for the dynamic import expression:

lib/plugins/utils.js:52:43
  ✖  52:43  import() expressions are not supported yet.  node/no-unsupported-features/es-syntax

however, all tests, including integration tests pass in our supported node range from my checking. all scenarios within the dynamic import expression seem to be covered by tests as well.

based on mysticatea/eslint-plugin-node#250, it sounds like the rule does not report correctly for this situation and is unlikely to be fixed soon. i hate to disable a rule like this and lose protection from other scenarios, but i'm unsure of a better approach for now. i'm open to suggestions

…yntax` when loading plugins based on mysticatea/eslint-plugin-node#250

travi · 2022-09-17T04:12:02Z

i went ahead and disabled the rule for that particular usage, since it seems like the best option, but open to alternative suggestions

gr2m · 2022-09-17T21:09:49Z

i hate to disable a rule like this and lose protection from other scenarios, but i'm unsure of a better approach for now. i'm open to suggestions

+1 for disabling. We can create a follow up issue to review once we concluded the ESM transition?

Great work @travi 👏🏼

…oading plugins (semantic-release#2558)

github-actions · 2022-11-11T15:26:19Z

🎉 This PR is included in version 20.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2023-01-06T19:56:52Z

🎉 This PR is included in version 20.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

refactor(plugins): switched from require to await import() when l…

fbc528b

…oading plugins to isolate the conversion ahead of #2543

travi requested a review from gr2m September 12, 2022 04:44

gr2m reviewed Sep 12, 2022

View reviewed changes

travi marked this pull request as draft September 13, 2022 14:43

travi added 4 commits September 16, 2022 16:58

fix(plugins): referenced the default of the imported plugin

476e0bb

and fixed paren grouping

refactor(plugins): improved variable names within reduce loops

4fb3cd2

refactor(plugins): removed console.logs and resolved lint issues

47ca65c

Merge branch 'master' of github.com:semantic-release/semantic-release…

97fdd48

… into async-import

test(lint): disabled the check for `node/no-unsupported-features/es-s…

eeae4d3

…yntax` when loading plugins based on mysticatea/eslint-plugin-node#250

travi force-pushed the async-import branch from 8f2b51a to eeae4d3 Compare September 17, 2022 04:10

travi marked this pull request as ready for review September 17, 2022 04:12

gr2m merged commit 4cd3641 into master Sep 17, 2022

gr2m deleted the async-import branch September 17, 2022 21:10

adityahex27 pushed a commit to hextrust/semantic-release that referenced this pull request Oct 31, 2022

refactor(plugins): switched from require to await import() when l…

0465859

…oading plugins (semantic-release#2558)

github-actions bot added the released on @beta label Nov 11, 2022

github-actions bot added the released label Jan 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(plugins): switched from `require` to `await import()` when loading plugins #2558

refactor(plugins): switched from `require` to `await import()` when loading plugins #2558

travi commented Sep 12, 2022

travi commented Sep 12, 2022

travi commented Sep 12, 2022

gr2m left a comment

gr2m Sep 12, 2022

travi Sep 12, 2022

gr2m Sep 12, 2022

travi Sep 12, 2022

travi commented Sep 16, 2022

travi commented Sep 16, 2022

travi commented Sep 17, 2022 •

edited

gr2m commented Sep 17, 2022

github-actions bot commented Nov 11, 2022

github-actions bot commented Jan 6, 2023

refactor(plugins): switched from require to await import() when loading plugins #2558

refactor(plugins): switched from require to await import() when loading plugins #2558

Conversation

travi commented Sep 12, 2022

travi commented Sep 12, 2022

travi commented Sep 12, 2022

gr2m left a comment

Choose a reason for hiding this comment

gr2m Sep 12, 2022

Choose a reason for hiding this comment

travi Sep 12, 2022

Choose a reason for hiding this comment

gr2m Sep 12, 2022

Choose a reason for hiding this comment

travi Sep 12, 2022

Choose a reason for hiding this comment

travi commented Sep 16, 2022

travi commented Sep 16, 2022

travi commented Sep 17, 2022 • edited

gr2m commented Sep 17, 2022

github-actions bot commented Nov 11, 2022

github-actions bot commented Jan 6, 2023

refactor(plugins): switched from `require` to `await import()` when loading plugins #2558

refactor(plugins): switched from `require` to `await import()` when loading plugins #2558

travi commented Sep 17, 2022 •

edited