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

refactor!: Convert all rules to ESM #535

Merged
merged 9 commits into from
Jul 2, 2023
Merged

Conversation

azu
Copy link
Member

@azu azu commented Jul 2, 2023

Changes

  • Change rollup config
  • Improve AggregationError message

Internal

  • New Internal package
    • @secretlint/secretlint-rule-internal-test-esm
    • @secretlint/secretlint-rule-internal-test-cjs
  • Add new testing way for preset

Note

  • rollup's @rollup/plugin-commonjs treat __esModule flag by default
  • However, Node.js ESM does not treat __esModule.
  • It is incompatible. We need to set commonjs({ defaultIsModuleExports: true }) to rollup.
  • Add various snapshot testing

@azu azu linked an issue Jul 2, 2023 that may be closed by this pull request
20 tasks
Comment on lines +47 to +50
}), resolve({ preferBuiltins: false }), commonjs({
// disable __esModule interop
// Node.js ESM does not recognize `__esModule` flag interop
defaultIsModuleExports: true
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 is incomatible issue between Node.js ESM and Rollup.

Comment on lines +168 to +181
const newError =
error instanceof Error
? new Error(
`Failed to load rule module: ${configDescriptorRule.id}

Error: ${error.message}`,
{
cause: error,
}
)
: new Error(`Failed to load rule module: ${configDescriptorRule.id}

Error: ${String(error)}`);
errors.push(newError);
Copy link
Member Author

Choose a reason for hiding this comment

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

Improve loading error.

@azu azu changed the title refactor: Convert all rules to ESM refactor!: Convert all rules to ESM Jul 2, 2023
@azu azu added the Type: Breaking Change Includes breaking changes label Jul 2, 2023
@@ -4,13 +4,21 @@ import commonjs from "@rollup/plugin-commonjs";

export default {
input: "src/index.ts",
// gcp rule is broken by moduleSideEffects option
// treeshake: "smallest",
Copy link
Member Author

Choose a reason for hiding this comment

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

    treeshake: {
        /**
         * "smallest" will choose option values for you to minimize output size as much as possible. This should work for most code bases as long as you do not rely on certain patterns, which are currently:
         *
         *     getters with side effects will only be retained if the return value is used (treeshake.propertyReadSideEffects: false)
         *     code from imported modules will only be retained if at least one exported value is used (treeshake.moduleSideEffects: false)
         *     you should not bundle polyfills that rely on detecting broken builtins (treeshake.tryCatchDeoptimization: false)
         *     some semantic issues may be swallowed (treeshake.unknownGlobalSideEffects: false, treeshake.correctVarValueBeforeDeclaration: false)
         */
        propertyReadSideEffects: false,
        tryCatchDeoptimization: false,
        unknownGlobalSideEffects: false,
        correctVarValueBeforeDeclaration: false,
        moduleSideEffects: (id, external) => {
            // node-fetch does not work
            if (id.includes("node_modules/node-forge")) {
                return true;
            }
            return false;
        }
    }

works, but file size is large.
node-forge is big.

@azu azu merged commit 77646c2 into master Jul 2, 2023
16 checks passed
@azu azu deleted the 530-rules-convert-to-esm-1 branch July 2, 2023 10:48
@github-actions github-actions bot mentioned this pull request Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change Includes breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rules: Convert to ESM
1 participant