Support webpack promise config and esm config files#774
Support webpack promise config and esm config files#774FlorianRappl merged 4 commits intosmapiot:developfrom
Conversation
|
|
File size impactImpact on file sizes when merging feat/webpack-promise-config into develop. empty-piral (no impact)
minimal-piral (no impact)
sample-cross-fx (no impact)
sample-piral (no impact)
sample-piral-core (no impact)
|
| // support Promise for returned config: | ||
| // https://webpack.js.org/configuration/configuration-types/#exporting-a-promise | ||
| if (configResult instanceof Promise) { | ||
| webPackConfig = await configResult; |
There was a problem hiding this comment.
I think this is unnecessary. We can just await configResult independent if its a Promise or not. JavaScript await is lax here, it will just work:
await 5; // 5
await Promise.resolve(5); // 5
await thisPromiseYieldsFive; // 5is all equivalent; so no need to have the instanceof check.
There was a problem hiding this comment.
That's true, I felt this approach was more explicit in intention. Sometimes developers accidentally await non-Promises and another dev comes by and fixes it by removing the await ... I wanted to make it clear that even though in 90% of cases this won't be a promise, the await should be preserved for a reason and was not a mistake.
The comment alone does this, but not everybody reads the docs!
There was a problem hiding this comment.
Sure I see the intention - I just want to make sure the code remains easy to digest and does not do unnecessary detours.
Also just a remark: The CLA was not signed as the associated email address does not seem to be registered with your account. You'll either need to add this email address or re-do commits with a different config using an email that you either have or can associate with your GitHub account.
There was a problem hiding this comment.
Understood, I'll make that change for you.
Yeah, I understand the CLA. I'm a new employee at my workplace so I'm tracking down our OSS contribution process.
There was a problem hiding this comment.
Any news on this one? Otherwise I'd close and re-do.
| if (existsSync(otherConfigPath)) { | ||
| try { | ||
| const otherConfig = require(otherConfigPath); | ||
| let otherConfig = require(otherConfigPath); |
There was a problem hiding this comment.
Question here: Would it make sense to check / support mjs extension here? And maybe use import(...) instead of require(...) when an mjs file is found?
There was a problem hiding this comment.
Could do that, although the CJS/ESM interop seems to work well enough here, and you'd still want it for plain .js cases, so I'd say it's more concise to leave this as-is.
FlorianRappl
left a comment
FlorianRappl
left a comment
There was a problem hiding this comment.
I like it - presumably other bundler integrations should support Promise results, too.
Thanks!
FlorianRappl
added
cli
|
Just checking in, I'm still navigating our legal department to confirm the CLA. Will update as soon as I have an answer. |
|
Ok, I got approval and the CLA has been signed. |
|
Awesome - I'll try to wrap up the 1.9.0 and release it this week. |
3 participants
New Pull Request
For more information, see the
CONTRIBUTINGguide.Prerequisites
Please make sure you can check the following boxes:
Type(s) of Changes
Contribution Type
What types of changes does your code introduce? Put an
xin all the boxes that apply:Description
Updates
piral-cli-webpack5to support the Promise-based Webpack config format.Remarks
Using a Promise to resolve the final config is niche but necessary for integrating ESM-only plugins, like UnoCSS
I have linked and tested
piral-cli-webpack5with a local project which uses async Webpack config and ESM module syntax (export default config) andpiral devruns and transpiles my app successfully with the correct modified config.