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 normalize after build #4972

Closed
wants to merge 1 commit into from

Conversation

bestlyg
Copy link
Contributor

@bestlyg bestlyg commented Nov 20, 2023

Summary

🤖[deprecated] Generated by Copilot at 32b2dba

Refactored the build config merging and normalization process in module-tools to improve performance and extensibility. Changed the beforeBuild hook context to use the merged config array instead of the normalized config object. Updated the build function in build.ts to use the new config structure and validation.

Details

🤖[deprecated] Generated by Copilot at 32b2dba

  • Simplify and improve the config merging and normalization logic for the build function in build.ts (link, link, link, link)
  • Use the new mergeBuildConfig function to combine the user config and the preset config before running the beforeBuild hooks (link, link)
  • Pass the merged config as an argument to the normalizeBuildConfig function after the hooks, and validate and transform it into the final config (link, link)
  • Change the type of the config property in the beforeBuild hook context to match the merged config structure (link)
import { moduleTools, defineConfig } from '@modern-js/module-tools';

const CWD = process.cwd();
const defaultConfig = [
  {
    sourceMap: 'external',
    target: 'es6',
    buildType: 'bundleless',
    format: 'cjs',
    outDir: './lib',
    copy: {
      patterns: [{ from: './src/style', to: './style', context: CWD }],
    },
  },
  {
    sourceMap: 'external',
    target: 'es6',
    buildType: 'bundleless',
    format: 'esm',
    outDir: './es',
    copy: {
      patterns: [{ from: './src/style', to: './style', context: CWD }],
    },
  },
];

const customPlugins: any = {
  setup() {
    return {
      beforeBuild({ config, cliOptions }) {
        console.log('beforeBuild', config, cliOptions);
        config.length = [];
        config.push(...defaultConfig);
      },
    };
  },
};

export default defineConfig({
  plugins: [moduleTools(), customPlugins],
  buildPreset: 'npm-library',
});

Hi.
I want to define a plugin that will clear all original config and insert some custom config.
But it dosen't work.
image
I look up the code and find the reason is the config only normalize before the beforeBuild hook.
It caused me to not be able to insert some custom config.
So I want to change some code that make normalize config after the beforeBuild hook.

Related Issue

Checklist

  • I have added changeset via pnpm run change.
  • I have updated the documentation.
  • I have added tests to cover my changes.

Copy link

changeset-bot bot commented Nov 20, 2023

⚠️ No Changeset found

Latest commit: 32b2dba

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@chenjiahan chenjiahan requested a review from 10Derozan November 21, 2023 07:21
@chenjiahan
Copy link
Member

@10Derozan Please review this PR ❤️

@10Derozan
Copy link
Contributor

10Derozan commented Nov 21, 2023

OK, I get it. In fact, I hope we can change the config by beforeBuild hook, so I try to use change beforeBuild to waterfall hook, but the waterfall hook require params idempotent, it failed finally.

In your case, you can remove buildPreset, this's what you want to delete, and then add your custom config.

This feature will break the behaviour about the beforeBuild params. It doesn't disrupt existing functionality, but the params are ambiguous.What do you think? @targeral

@bestlyg
Copy link
Contributor Author

bestlyg commented Nov 21, 2023

OK, I get it. In fact, I hope we can change the config by beforeBuild hook, so I try to use change beforeBuild to waterfall hook, but the waterfall hook require params idempotent, it failed finally.

In your case, you can remove buildPreset, this's what you want to delete, and then add your custom config.

This feature will break the behaviour about the beforeBuild params. It doesn't disrupt existing functionality, but the params are ambiguous.What do you think? @targeral

Hi, thank you for your reply.

I want to use a plugin to unify all my configuration changes, because I may not use it in my project repository.

It may be different from the existing logic, because the existing logic will normalize before beforeBuild.
I think we can normalize it before beforeBuild.
It will normalize twice, but the logic stays the same.

@bestlyg
Copy link
Contributor Author

bestlyg commented Nov 21, 2023

OK, I get it. In fact, I hope we can change the config by beforeBuild hook, so I try to use change beforeBuild to waterfall hook, but the waterfall hook require params idempotent, it failed finally.

In your case, you can remove buildPreset, this's what you want to delete, and then add your custom config.

This feature will break the behaviour about the beforeBuild params. It doesn't disrupt existing functionality, but the params are ambiguous.What do you think? @targeral

I agree you. we need change beforeBuild to waterfall.
When multiple plugins are used, they may produce different results due to some asynchronous timing issues.

@10Derozan
Copy link
Contributor

In fact, we have a config hook, unfortunately, this hook was not designed to override the user's configuration file, it seems we can only add a hook to solve this problem.

@10Derozan
Copy link
Contributor

I will add a hook named beforeNormalizedConfig, which type is AsyncWaterfall. Everyone can change config by this hook.
And the merge operation is still inside normalize, which is a built-in operation.

@bestlyg
Copy link
Contributor Author

bestlyg commented Nov 21, 2023

I will add a hook named beforeNormalizedConfig, which type is AsyncWaterfall. Everyone can change config by this hook. And the merge operation is still inside normalize, which is a built-in operation.

thanks, it will also slove my problem.

@10Derozan
Copy link
Contributor

Thanks for understanding, this will not break any existing functionality.

I will add a hook named beforeNormalizedConfig, which type is AsyncWaterfall. Everyone can change config by this hook. And the merge operation is still inside normalize, which is a built-in operation.

thanks, it will also slove my problem.

@10Derozan
Copy link
Contributor

I'll close this, you can raise a suggestion in #4982

@10Derozan 10Derozan closed this Nov 21, 2023
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.

3 participants