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

support builtin:swc-loader with custom swc options #3067

Closed
Tracked by #3309
hardfist opened this issue May 9, 2023 · 31 comments · Fixed by #3382
Closed
Tracked by #3309

support builtin:swc-loader with custom swc options #3067

hardfist opened this issue May 9, 2023 · 31 comments · Fixed by #3382
Assignees
Labels
feat New feature or request need documentation Create a tracking issue in rspack-website team The issue/pr is created by the member of Rspack.

Comments

@hardfist
Copy link
Contributor

hardfist commented May 9, 2023

What problem does this feature solve?

users need more and more swc custom options now (see #3054) , and it's not ideal to put more and more swc options to builtins which will bloat builtins, so it would be better to put all these custom swc options to loader, once we support builtin-swc-loader, we may deprecate type: 'ts' and the default transformation of javascript module in the future, which makes rspack more compatible with webpack

The builtin:swc-loader is much faster than the js version of https://swc.rs/docs/usage/swc-loader because it can run parallel in rust side and it avoids the rust js communication

What does the proposed API of configuration look like?

module.exports = {
  module: {
   rules: [{
      test: /\.js/,
      use: {
           loader: 'builtin:swc-loader',
           options: {

                        parser: {
                            syntax: "typescript"
                        },
                        transformer: {
                            useDefineForClassFields: true
                       }
                }
      }
   }]
 }
}
@hardfist hardfist added feat New feature or request pending triage The issue/PR is currently untouched. labels May 9, 2023
@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label May 9, 2023
@hardfist hardfist added p1-signifincant-downstream-blocker and removed pending triage The issue/PR is currently untouched. labels May 16, 2023
@h-a-n-a h-a-n-a added this to the Planned milestone May 16, 2023
@h-a-n-a h-a-n-a self-assigned this May 16, 2023
@jerrykingxyz jerrykingxyz modified the milestones: Planned, 0.1.14 - Patch May 23, 2023
@h-a-n-a h-a-n-a assigned suxin2017 and unassigned h-a-n-a Jun 6, 2023
@hyf0 hyf0 removed this from the 0.1.14 - Patch milestone Jun 7, 2023
@hardfist
Copy link
Contributor Author

hardfist commented Jun 7, 2023

we may need a option to disable default transformation of javascript | typescript module, since it will cause the module is transformed by swc twice once people use swc-loader, but the default transformation behavior is friendly to user than manually configure loader especially for new user.
one solution is we introduce a concept called preset(which I remember @TheLarkInn mentioned before), which controlled whether rspack will transform these modules by default

{
  presets: 'default'  | 'transform' // default means don't do any transformation by default and 'transform' means transform ts and js using swc by default
}

@TheLarkInn @alexander-akait what's the plan of the preset concept and any suggestions?

@alexander-akait
Copy link

@hardfist we are still considering this concept (and I don't think preset will sove this problem fully), but there are already two solutions:

  1. pass AST from swc-loader, so rspack can avoid double parsing, webpack supports it, but only for estree compatibility parsers
  2. redefine defaultRules and use swc-loader by default

I think the valid usage is the first, because you can't neven know which options will be used, but if you have AST, you don't need to parse everything twicy, the same can be used for CSS and HTML, unfortunately, no estree analog for CSS and HTML, but I for HTML it is not a problem, because swc-html uses the browser logic and create DOM, so it can be used as a standard, for CSS it is tricky, anyway right now I have dicussion in stylleint about full features parser and analog for CSS, so it will allow to reuse CSS AST across different parsers.

Here https://github.com/webpack/webpack/blob/main/lib/javascript/JavascriptParser.js#L3897 usage of AST
Anyway we use this approach even for custom parsers, for example https://github.com/webpack-contrib/postcss-loader/blob/master/src/index.js#L228, we pass AST from postcss-loader to css-loader and avoid multiple parsing

@hardfist
Copy link
Contributor Author

hardfist commented Jun 8, 2023

pass AST from swc-loader, so rspack can avoid double parsing, webpack supports it, but only for estree compatibility parsers

we did think of passing ast from swc-loader to rspack to avoid parsing, the tricky part is how to deal with span(or called range), since weback dependency analysis relys on span, but when we use swc-transform the span is polluted so we have to do codegen and parse again to get the right span, so unless swc-transform can ensure span is right after transformation(which I'm not sure is possible)
estree compatibility seems not a problem here, since builtin swc-loader and rspack are shipped together so we don't need to follow estree compatibility internally and we wouldn't need to reuse ast return from js loader(since the ast passing from js to rust have large cost)

@alexander-akait
Copy link

we did think of passing ast from swc-loader to rspack to avoid parsing, the tricky part is how to deal with span(or called range), since weback dependency analysis relys on span, but when we use swc-transform the span is polluted so we have to do codegen and parse again to get the right span, so unless swc-transform can ensure span is right after transformation(which I'm not sure is possible)

Yes, I understand your problem, I completely forgot that this does not happen to save time, although my personal opinion is that this is a bug, at least there should be an option to force it to do. Technically we should have AST = untransform(transform(AST)), but it is theoretical stuff.

Just side infromation - honestly i don't understand why swc-ecma-parser is not compatibility with estree, that would be just amazing, we can use it in many tools just like a replacement (yes it take some muck more times due since the ast passing from js to rust have large cost), but it will be allow to start smooth transitions for many tooling or at least provide an opportunity to use alternative approaches. Even more - 80-90 percents of the structures are already identical (at the last moment when I worked with swc).

So do you want to implement presets to hide implementations from users, right? Because developer can setup swc-loader using rules now.

@hardfist
Copy link
Contributor Author

hardfist commented Jun 8, 2023

So do you want to implement presets to hide implementations from users, right? Because developer can setup swc-loader using rules now.

I think defaultRules is good enough to handle compatibility now, so I don't think introduce preset before webpack does is a good idea, and in the long run we consider deprecate the whole builtins options and replace it with swc-loader with swcOptions and mock plugin this makes rspack more compatible with wepback.
but I'm still not sure should we keep things like type: ts, because although it's not compatible with webpack but it's user friendly for normal user but it's hard to work with swc-loader, I tend to deprecate it for compatibility

@hardfist
Copy link
Contributor Author

hardfist commented Jun 8, 2023

Here webpack/webpack@main/lib/javascript/JavascriptParser.js#L3897 usage of AST
Anyway we use this approach even for custom parsers, for example webpack-contrib/postcss-loader@master/src/index.js#L228, we pass AST from postcss-loader to css-loader and avoid multiple parsing

so this only works for style-loader or you need to do right span calculation in this postcss-loader?

@hardfist
Copy link
Contributor Author

hardfist commented Jun 8, 2023

talking about swc-loader, it opens the possibilities for user to do custom dynamic loaded plugin by swc wasm plugin, which maybe not stable enough, we can also implement wasm-loader(I'm not sure it'a good idea because even the string transformation is easy, but the loader Interface is complex)

@hardfist
Copy link
Contributor Author

hardfist commented Jun 8, 2023

Just side infromation - honestly i don't understand why swc-ecma-parser is not compatibility with estree, that would be just amazing, we can use it in many tools just like a replacement (yes it take some muck more times due since the ast passing from js to rust have large cost), but it will be allow to start smooth transitions for many tooling or at least provide an opportunity to use alternative approaches. Even more - 80-90 percents of the structures are already identical (at the last moment when I worked with swc).

I remember @Boshen's oxc is not compatible with estree too? @Boshen do you think estree compatible ast is a good idea for rust javascript parser?

Yes, I understand your problem, I completely forgot that this does not happen to save time, although my personal opinion is that this is a bug, at least there should be an option to force it to do. Technically we should have AST = untransform(transform(AST)), but it is theoretical stuff.

does babel-loader or swc-loader return ast now? I'm wondering whether babel-loader could get the right span after the transformation or does babel can provides things like untransform to get right span without reparsing

@alexander-akait
Copy link

alexander-akait commented Jun 8, 2023

so this only works for style-loader or you need to do right span calculation in this postcss-loader?

postcss always caculates spans, so there is no problems with it and yes, we don't use spans here, so I think we don't have such problems at all, we need structures like @import and url()

does babel-loader or swc-loader return ast now? I'm wondering whether babel-loader could get the right span after the transformation or does babel can provides things like untransform to get right span without reparsing

If we look at coode - no, but it was design for it, some decent time ago it was not compatible, but at the moment I don't know, we should check it out, but yeah, this feature was originally created for this, it is a pity that it has not gained popularity, because it makes eveything more faster

@hardfist
Copy link
Contributor Author

So do you want to implement presets to hide implementations from users, right? Because developer can setup swc-loader using rules now.

I think defaultRules is good enough to handle compatibility now, so I don't think introduce preset before webpack does is a good idea, and in the long run we consider deprecate the whole builtins options and replace it with swc-loader with swcOptions and mock plugin this makes rspack more compatible with wepback. but I'm still not sure should we keep things like type: ts, because although it's not compatible with webpack but it's user friendly for normal user but it's hard to work with swc-loader, I tend to deprecate it for compatibility

even defaultFules is good enough, but it's not that easy to manipulate the defaultRules, user may need to modify defaultRules config directly to remove the internal swc-loader which seems not ideal? @alexander-akait any suggestions?

@alexander-akait
Copy link

@hardfist hm, defaultFules was design for such purposes 😄 Can you show me what is hard there? Maybe you have a good idea (just an example if a configuration), so we can think about it, technically you also can implement custom type for modules, for example javascript/swc-esm/javascript/swc-dynamic and etc and put them as defaults, also developer always can use swc-loader with own options and also we build-in in the one project

@hardfist
Copy link
Contributor Author

hardfist commented Jun 12, 2023

@alexander-akait its easy to add but hard to delete,because users need to know the internal implementation to safely delete swc rule and not breaking other rule

@hardfist hardfist reopened this Jun 13, 2023
@alexander-akait
Copy link

alexander-akait commented Jun 13, 2023

@hardfist How you want to get AST and ranges from swc-loader? Because if it doesn't return ranges, we still need reparse it...
We can implement something like:

module.exports = {
  module: {
    parser: {
      defaultTypes: {
        javascript: "javascript/esm",
        // javascript: "javascript/esm+swc"
        css: "css/modules"
        // etc
      }
  },
};

@hardfist
Copy link
Contributor Author

hardfist commented Jun 13, 2023

@hardfist How you want to get AST and ranges from swc-loader? Because if it doesn't return ranges, we still need reparse it...

yeah as i said before we just give up reusing ast from swc-loader because of the transform range problems (but we still get performance gain since it's in rust and parallel)
we can only reuse ast from builtin:swc-loader when swc-transform can keep right range and reuse ast from js:swc-loader seems don't bring performance gains because of the rust-js communication cost

@hardfist
Copy link
Contributor Author

We can implement something like:

module.exports = {
  module: {
    parser: {
      defaultTypes: {
        javascript: "javascript/esm",
        // javascript: "javascript/esm+swc"
        css: "css/modules"
        // etc
      }
  },
};

the defaultTypes seems much more flexible than defaultRules and seems we can keep type:ts for this

@alexander-akait
Copy link

Yeah, we can implement this, it can create a weird case like javascript: "css" 😆 but I think people will not use it (also parser just failed because CSS parser can't parse Javascript)

@stale
Copy link

stale bot commented Aug 14, 2023

This issue has been automatically marked as stale because it has not had recent activity. If this issue is still affecting you, please leave any comment (for example, "bump"). We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the stale label Aug 14, 2023
@jraoult
Copy link

jraoult commented Aug 14, 2023

I stumbled upon this use while trying to find a solution to turn on useDefineForClassFields. I don't know if there is another way at this stage but it seemed the most promising.

@stale stale bot removed the stale label Aug 14, 2023
@hardfist
Copy link
Contributor Author

hardfist commented Aug 15, 2023

I stumbled upon this use while trying to find a solution to turn on useDefineForClassFields. I don't know if there is another way at this stage but it seemed the most promising.

it's designed to solve this problem, even it's completed right now but I think it can be used to solve your problem now by using swc-loader before like #3267 (comment), what's the blocker for you?

@jraoult
Copy link

jraoult commented Aug 15, 2023

EDIT: I realised this is not implemented yet. But this would allow me to configure SWC the way I need it.

Thanks for the pointer @hardfist. I'll try it when I have a moment. I need useDefineForClassFields to get Mobx to work properly (see https://mobx.js.org/installation.html#use-spec-compliant-transpilation-for-class-properties).

@hardfist
Copy link
Contributor Author

@jraoult it's actually implemented and you can use it now(a example here https://github.com/web-infra-dev/rspack/blob/main/examples/builtin-swc-loader/rspack.config.js#L12), what left is how we disable the default transformation so user's code don't need to transform twice(swc-loader and the default transform)

@thinkeyy
Copy link

Can I extend swc plugins in this way ? like:
jsc: { experimental: { plugins: [ ['swc-plugin-xxx', {}] ], },

@hardfist
Copy link
Contributor Author

hardfist commented Aug 16, 2023

Can I extend swc plugins in this way ? like: jsc: { experimental: { plugins: [ ['swc-plugin-xxx', {}] ], },

you mean the swc wasm plugin? we plan to support swc wasm plugin in the future, but since wasm plugin is experimental and swc is working on new wasm plugin https://twitter.com/kdy1dev/status/1689859032415358976, it's not on the top priority

@thinkeyy
Copy link

Thank you for your reply ,
I plan to migrate some old projects to rspack,In these old projects I used a custom babel plugin to help configure cssmodule automatically, the babel plugin transform

import a from 'foo.less';
Import 'foo.less';
import c from 'foo.sass';
import d from '.. /foo.css';

to

import a from "foo.less?modules";
Import 'foo.less'; 
import c from "foo.sass?modules";
import d from ".. /foo.css?modules";

Then , with webpack rule.resourceQuery(/modules/) open cssmodule。
Now , use repack, Do you have any suggestions for this

@hardfist
Copy link
Contributor Author

why not just configure rule directly

{
  module: {
   rule: {
      test: /\.less$/,
      use: [{loader:'css-module', options: {module:true}},'less-loader'],
   }
}
}

@thinkeyy
Copy link

The same as the demo above, when we use import 'foo.less', the CSS module is not enabled.. It's easier for users to maintain the global style. I also thought about automatically enabling the CSS module by using a file name suffix like 'index.module.less'. However, changing the file names manually would be too expensive。🤔

@hardfist
Copy link
Contributor Author

The same as the demo above, when we use import 'foo.less', the CSS module is not enabled.. It's easier for users to maintain the global style. I also thought about automatically enabling the CSS module by using a file name suffix like 'index.module.less'. However, changing the file names manually would be too expensive。🤔

can you provide a repro?

@9aoy
Copy link
Collaborator

9aoy commented Aug 17, 2023

@thinkey-yang resourceQuery is also supported in Rspack, it seems that you can write like this:

    rule: {
      test: /\.less$/,
      oneOf: [
        {
          resourceQuery: /modules/,
          type: 'css/module',
          use: [
            // xxx
          ],
        },
        {
          type: 'css',
          use: [
            // xxx
          ],
        },
      ],
    },

@thinkeyy
Copy link

@hardfist hardfist added the need documentation Create a tracking issue in rspack-website label Aug 17, 2023
@hardfist
Copy link
Contributor Author

@hardfist my project is build by umi , you can get more info form : umijs/swc-plugin-auto-css-modules and umijs/umi@3.x/packages/bundler-webpack/src/getConfig/css.ts.

to be honest this is a bad design of umi, we have no plan to support this bad design, we suggest you configure css module by module.rules other than auto detect in import style in module.

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Aug 22, 2023

Closed as it has been supported. Let's move the further questions to another issue or here: #3067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request need documentation Create a tracking issue in rspack-website team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants