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(cli): support for .cts.mts and follows type: module #3222

Merged
merged 13 commits into from
Jun 23, 2023

Conversation

molvqingtai
Copy link
Contributor

@molvqingtai molvqingtai commented May 18, 2023

Related issue

Enhanced configuration files, added support for .cts and .mts, and fixed the problem of not following the user type: module.

summary

cli will automatically register loader for .ts | .cts files, for .ts files marked with type: nodule or .mts will be considered as ESM and need to use cli to specify the loader.

Walkthrough

rspack.config.ts:

import path from 'node:path'
import { fileURLToPath } from 'node:url'

const __dirname = path.dirname(fileURLToPath(import.meta.url))

export default {
  mode: 'production',
  entry: path.resolve(__dirname, 'src/index.ts'),
  output: {
    path: path.resolve(__dirname, 'dist')
  }
}

package.json

{
  "type": "module",
  "scripts": {
    "build": "node --loader ts-node/esm rspack build -c rspack.config.ts",
  },
  "devDependencies": {
    "@types/node": "^20.3.1",
    "ts-node": "^10.9.1",
    "typescript": "^5.1.3"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "allowImportingTsExtensions": true,
    "noEmit": true
  }
}

It should work. 🎉

@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
All committers have signed the CLA.

@molvqingtai molvqingtai changed the title feat: [cli] The configuration file follows the type property of package.json. feat: [cli] The configuration file follows the “type“ property of package.json. May 18, 2023
@molvqingtai molvqingtai changed the title feat: [cli] The configuration file follows the “type“ property of package.json. feat: [cli] The configuration file follows the "type" property of package.json. May 18, 2023
@hyf0 hyf0 requested a review from hardfist May 19, 2023 01:25
@hardfist
Copy link
Contributor

hardfist commented May 19, 2023

we discussed how to support .ts in #2237 and decide to use transform strategy other than bundle strategy, so use esbuild to prebundle config is not ideal

@molvqingtai
Copy link
Contributor Author

we discussed how to support .ts in #2237 and decide to use transform strategy other than bundle strategy, so use esbuild to prebundle config is not ideal

I tried to use rspack self-compilation in the code, but the output file seems to have a problem and cannot be imported or required correctly.

https://github.com/molvqingtai/rspack/blob/main/packages/rspack-cli/src/utils/loadConfig.ts#L32

@hardfist
Copy link
Contributor

I tried to use rspack self-compilation in the code, but the output file seems to have a problem and cannot be imported or required correctly.

use rspack to bundle config is not ideal which is disscussed in #2237

@hardfist
Copy link
Contributor

it's a BUG that rspack not supports "type" property of packageJson, but it should not be fixed by bundle config file

@molvqingtai
Copy link
Contributor Author

it's a BUG that rspack not supports "type" property of packageJson, but it should not be fixed by bundle config file

Is it necessary to fallback to ts-node register and only add type: "module" detection and .cts .mts support?
Because in the project, all files are default esm, the .mts configuration file looks ugly. 😁

@hardfist
Copy link
Contributor

i think this should break into two parts type field support and mts|cts support,and we dont plan to support mts|cts

@molvqingtai
Copy link
Contributor Author

i think this should break into two parts type field support and mts|cts support,and we dont plan to support mts|cts

i think this should break into two parts type field support and mts|cts support,and we dont plan to support mts|cts

Why not? Is it because of the complexity of refactoring into a transform strategy in the future?

@molvqingtai molvqingtai changed the title feat: [cli] The configuration file follows the "type" property of package.json. refactor: [cli] Load the configuration file using jiti May 26, 2023
@molvqingtai
Copy link
Contributor Author

molvqingtai commented May 26, 2023

we discussed how to support .ts in #2237 and decide to use transform strategy other than bundle strategy, so use esbuild to prebundle config is not ideal

Hey, I have refactored the code and updated the documentation. I used jiti without bundling packages, and the dependency issue mentioned in #2237 has been resolved.

I tested jiti because it only runs the config file, so it's very fast, like node VM, unlike ts-node which pollutes the global scope.

@molvqingtai molvqingtai changed the title refactor: [cli] Load the configuration file using jiti refactor(cli): Load the configuration file using jiti runtime. May 27, 2023
@hardfist
Copy link
Contributor

hardfist commented May 31, 2023

I tested jiti because it only runs the config file, so it's very fast, like node VM, unlike ts-node which pollutes the global scope.

sorry it seems webpack doesn't support mts | cts | ts with type:module either, and this implementation is too complicated and introduces tons of dependency(jiti uses babel) and edge case.
if you need to use mts | cts | ts with type:module i think you could use jiti on userland

@alexander-akait does webpack have any plan to support mts | cts | ts with type:module config?

@alexander-akait
Copy link

@hardfist

@alexander-akait does webpack have any plan to support mts | cts | ts with type:module config?

I thought about it, but I still think that it should be on the user side, there are a lot of different tools and approaches, but I opened to discussion

@hardfist
Copy link
Contributor

webpack/webpack-cli#3831 since webpack has supported mts|cts now, we may support it as well, @molvqingtai are you still willing to work on this?

@alexander-akait
Copy link

@hardfist note - we don't use jiti, we allow to use any tools i.e. ts-node, babel and etc

@molvqingtai
Copy link
Contributor Author

@hardfist note - we don't use jiti, we allow to use any tools i.e. ts-node, babel and etc

Let the user specify node loader like webpack L11, I don't think this is a good idea.

Use babel or other transform? I don't have a good idea at present.

@alexander-akait
Copy link

@molvqingtai

Let the user specify node loader like webpack L11, I don't think this is a good idea.

Why? You can't handle all possible tools and their output

@molvqingtai
Copy link
Contributor Author

@molvqingtai

Let the user specify node loader like webpack L11, I don't think this is a good idea.

Why? You can't handle all possible tools and their output

ESM/CJS compatibility issues should not be thrown to the user. As you can imagine, when the user does not specify a loader and causes an error, it will be a bad experience to search the document for a solution.

@molvqingtai
Copy link
Contributor Author

@molvqingtai

Let the user specify node loader like webpack L11, I don't think this is a good idea.

Why? You can't handle all possible tools and their output

ESM/CJS compatibility issues should not be thrown to the user. As you can imagine, when the user does not specify a loader and causes an error, it will be a bad experience to search the document for a solution.

Using jiit is currently the simplest solution while maintaining a good DX. As for edge cases, both nuxt (nuxt/vite#201) and tailwindcss(tailwindlabs/tailwindcss#10785) are jiti to load configuration files. I believe this solution is stable enough.

@alexander-akait
Copy link

alexander-akait commented Jun 12, 2023

@molvqingtai I strongly disagree, but I will not argue, we already tried to do it and faced with a lot of problems when new things appear (syntax/different outputs/just features like swc for ts-node/ES modules), why I should use babel (and have dependecies) when I want to use swc. Setting up the runtime environment is just the task of the developer and no one else, you can use jiti under the hood if nothing was configured (or no options/setting provided), but always using and not allowing customization is a pretty bad idea, you can of course not take someone else's experience and go through the same problems again, your choice

@alexander-akait
Copy link

And yes, don't forget about performance, babel is not fast

@molvqingtai
Copy link
Contributor Author

And yes, don't forget about performance, babel is not fast

Of course, you can use swc. tailwindlabs/tailwindcss@f1614c8

@alexander-akait
Copy link

alexander-akait commented Jun 12, 2023

@molvqingtai What about another solutions? Want to say - some big (and not only big) tech companies use forks of tools or self created (with remote cache and etc features), if you don't provide them to use it, it will be a blocker, again - you should allow to use custom tools

@hardfist
Copy link
Contributor

@molvqingtai I'll keep fixing the left issue to get this pr merged ASAP if you don't mind

@hardfist
Copy link
Contributor

closes #3609

@molvqingtai
Copy link
Contributor Author

@molvqingtai I'll keep fixing the left issue to get this pr merged ASAP if you don't mind

Of course

@hardfist hardfist enabled auto-merge June 23, 2023 04:04
@hardfist
Copy link
Contributor

!canary

@hardfist hardfist added this pull request to the merge queue Jun 23, 2023
Merged via the queue into web-infra-dev:main with commit 955d01c Jun 23, 2023
@hardfist
Copy link
Contributor

@molvqingtai thank you for your great work

@hardfist hardfist added need documentation Create a tracking issue in rspack-website and removed need documentation Create a tracking issue in rspack-website labels Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need documentation Create a tracking issue in rspack-website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants