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(config-loader): support a rule written by ESM #187

Merged
merged 7 commits into from Sep 14, 2021
Merged

feat(config-loader): support a rule written by ESM #187

merged 7 commits into from Sep 14, 2021

Conversation

azu
Copy link
Member

@azu azu commented Sep 12, 2021

Currently, secretlint can not load a rule that is written by ESM.

This PR aims to allow to load secretlint rule as ESM modules.

Blocker

TypeScript does not support import() in CommonJS

__esModule interop is broken.
We need to pick .default.default. It is ugly.

import { validateConfig, validateRawConfig } from "@secretlint/config-validator";

export function moduleInterop<T>(moduleExports: T): T {
if (moduleExports && (moduleExports as any).__esModule) {
return (moduleExports as any).default?.default!;
Copy link
Member Author

@azu azu Sep 12, 2021

Choose a reason for hiding this comment

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

module.exports.default = 1

is default.default.
__esModule is broken mechanism in real CJS/ESM interop.
image

@jimmywarting
Copy link

jimmywarting commented Sep 13, 2021

tip: just something that helped me with async imports:
TypeScript/WebPack rewrites my async imports to require in CJS projects

const _importDynamic = new Function('modulePath', 'return import(modulePath)')
await _importDynamic('node-fetch') // v3 is esm only


// FIXME: https://github.com/microsoft/TypeScript/issues/43329
// module: node12 will be replace it
const _importDynamic = new Function("modulePath", "return import(modulePath)");
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This workaround should be removed in the future

  • CSP Issue
  • Performance Issue
  • Type Issue

@azu
Copy link
Member Author

azu commented Sep 13, 2021

Yes. We have used _importDynamic workaround.

@azu azu mentioned this pull request Sep 14, 2021
@azu azu merged commit 590c333 into master Sep 14, 2021
@azu azu deleted the node-esm branch September 14, 2021 11:34
@@ -123,7 +120,7 @@ export const loadPackagesFromRawConfig = (
// TODO: any to be remove
const ruleModule: any = replacedDefinition
? replacedDefinition.rule
: moduleInterop(require(moduleResolver.resolveRulePackageName(configDescriptorRule.id)));
: moduleInterop(await _importDynamic(moduleResolver.resolveRulePackageName(configDescriptorRule.id)));
Copy link
Member Author

Choose a reason for hiding this comment

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

It cause a bug #205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants