dilanx / craco

Create React App Configuration Override, an easy and comprehensible configuration layer for Create React App.
https://craco.js.org
Apache License 2.0
7.44k stars 499 forks source link

Allow craco config to be written in Typescript #290

Closed IronSean closed 2 years ago

IronSean commented 3 years ago

Allowing the craco.config.js file to be a craco.config.ts file, and defining a type for the config object would be a nice addition. It would also allow plugin makes to type their plugins.

235 was about this too but was closed without any response.

patricklafrance commented 3 years ago

Hi,

If someone is willing to send a PR, I am willing to review this PR.

alvis commented 3 years ago

@patricklafrance I just made a PR #334 to resolve this issue. Please help for a cross-check ;)

patricklafrance commented 3 years ago

Will have a look this weekend @alvis thank you

mrlubos commented 2 years ago

Hey @alvis @patricklafrance, I didn't dig too deep into it, but it appears to me that converting a craco.config.js file that looks like this

module.exports = () => ({
  plugins: [
    {
      options: someOptions,
      plugin: somePlugin,
    },
  ],
});

into a craco.config.ts file that looks like this

export default () => ({
  plugins: [
    {
      options: someOptions,
      plugin: somePlugin,
    },
  ],
});

will not work. Instead, I have to get rid of the wrapping function so the craco.config.ts file ends up looking like this

export default {
  plugins: [
    {
      options: someOptions,
      plugin: somePlugin,
    },
  ],
};

It is possible this bug happens due to my project configuration, but I can't tell unless more people report this issue.

The problem occurs in the getConfigAsObject() function where the isFunction(result.config) condition returns false for the second code block above. This is because result is a promise in that instance, so it should be resolved first and only then you'd get the original function that should be tested in the condition.

I am using ts-node to execute a TypeScript file that uses child_process to run the craco CLI tool. I am not sure if my tooling impacts anything, but the result constant ends up being a promise in my case. Please let me know if I can be any more helpful with this and thank you for adding the TypeScript functionality!

EDIT: I should add that this bug happens silently, my config gets simply ignored, there are no errors thrown. The solution above (third code block) works for my use case so it's not a blocker for me, but might be worth investigating.

alvis commented 2 years ago

@mrlubos Thanks for reporting. It appears that it's an upstream issue https://github.com/EndemolShineGroup/cosmiconfig-typescript-loader/issues/134 which actually has been fixed by https://github.com/achmadk/cosmiconfig-typescript-loader/commit/af16119b6a924a9f117e72375cdb7694353c24e3.

However, the author didn't make a PR so the fix is not available today. Even a PR is made, I'm not very confident that it will be released in a timely manner given the PR activity.

However, having said that, the loader script is not complicated at all. It's only 18 lines including line spaces. So we could just embed the code easily as it's written under MIT License.

@patricklafrance what do you think?

patricklafrance commented 2 years ago

Hey @alvis

Sounds good, as long as the duplicated code is scoped somewhere and it's easy to understand it's temporary and where the original code is at.

Thanks

alvis commented 2 years ago

@patricklafrance A proper fix is now ready to be reviewed. ;)