swc icon indicating copy to clipboard operation
swc copied to clipboard

Config files fail with top-level array

Open jrieken opened this issue 3 years ago • 16 comments

Describe the bug

According to the docs and this discussion it should be possible to have an array as config but SWC fails

Playground steps

  • open playground
  • replace the config with the snippet below
  • 🔥 the page crashes

Input code

;

Config

[
  {
    "$schema": "http://json.schemastore.org/swcrc"
  }
]

Playground link

No response

Expected behavior

SWC should pick up each element of the config-array as separate configuration

Actual behavior

It errors, playground crashes

Steps

Version

1.2.246

Additional context

No response

jrieken avatar Sep 02 '22 08:09 jrieken

Alternative/real-world steps:

  • clone https://github.com/microsoft/vscode/tree/joh/swc-array-config
  • run npx swc --config-file ./build/lib/swc/.swcrc-all --out-dir ./out2 ./src

jrieken avatar Sep 02 '22 08:09 jrieken

I gave it a quick go on this and found out top-level array of config was never consistently supported. In some part of swc consumes it, but mostly betrays what people assumes.

  1. @swc/core bindings api

transform(code, { configFile: ... }) works fine, but if configFile is top-level array, it seems being ignored simply. After all, transform* interface never had multiple output return value but only supports single TransformOutput makes things bit tricker.

If .swcrc contains multiple config, should it return Array<TransformOutput> instead? That's probably what most people presume to work. But this is slight possibility of breaking changes for the interface since it no longer explicitly returns single output. We can't have reliable typescript inference on this one, since configFile is a path to the string doesn't have any compile time way to figure out if it's multiple or not.

  1. @swc/cli

Fails to parse.

failed to process input file

Caused by:
    0: failed to read swcrc file (index.js)
    1: failed to read config (.swcrc) file
    2: No such file or directory (os error 2)
Error: Failed to compile 1 file with swc.
    at Object.assertCompilationResult (/Users/ojkwon/github/swc/node_modules/@swc/cli/lib/swc/util.js:113:15)
    at files (/Users/ojkwon/github/swc/node_modules/@swc/cli/lib/swc/file.js:173:18)
    at async _default (/Users/ojkwon/github/swc/node_modules/@swc/cli/lib/swc/file.js:192:9)
  1. swcx

Fails to parse.

Error: failed to process input file

Caused by:
    0: failed to read swcrc file (./index.js)
    1: .swcrc exists but not matched

I think at this point it's safe to assume this feature is non-existent (or not working at all) and design interface again from scratch. Probably biggest question is how can we provide consistence between binding interfaces vs. cli, as well as should we perform single-input multiple-output transforms (which we never did & generally avoid).

Or simply, we take this back and enforce .swcrc to not allow arrays. It's simple, but breaking changes, and possibly disappoint @jrieken 😅 .

/cc @kdy1 for thought, as well as @jrieken if you can share some opinions. Specifically, how critical it is to have these features? I'm bit concerned this could be a major surgery to existing interfaces (`transform* and other bindings interfaces) and curious if we could achieve similar without those.

kwonoj avatar Sep 03 '22 02:09 kwonoj

I'm not sure how critical it is, but I prefer to fix the array config because we can avoid a breaking change. Not sure how much will it take, though

kdy1 avatar Sep 03 '22 05:09 kdy1

I prefer to fix the array config because we can avoid a breaking change

Is there a way to make non-breaking changes for this?

for example, let's think about this

transform(): Promise<TransformOutput>

now becomes

transform(): Promise<Array<TransformOutput>>

if we'd like to allow multiple config input via configfile, unless we decide to omitting non-first config in the swcrc.

In the opposite, if we decide to keep transform api as-is and cut off array support, it's also obvious breaking changes.

kwonoj avatar Sep 03 '22 05:09 kwonoj

The top-level array in .swcrc is not about emitting multiple files at once. It's about using different configuration based on patterns.

See https://swc.rs/docs/configuration/compilation#multiple-entries

So we don't need to change transform()

kdy1 avatar Sep 03 '22 05:09 kdy1

based on patterns

But patterns can be non-exclusive, in that case how will it works? if config A and B specifies both file, then we'll just take one? and should we calculate it per each transform? Also, I'd like to point out that's somewhat non-intuitive for some users, as they might expect (or misunderstood) like me to behave single-input to multiple outputs.

So far, in my opinion this seems a source of confusion, including current design and implementation.

kwonoj avatar Sep 03 '22 05:09 kwonoj

Yeah, the first one is used. Honestly, I don't think people will expect it to emit multiple files at once, but I think we need to refactor this with the next breaking change (v2)

kdy1 avatar Sep 03 '22 05:09 kdy1

I don't think people will expect it to emit multiple files at once,

The way I understood was actually in this way, also @jrieken should double confirm but if you see vscode's config (https://github.com/microsoft/vscode/blob/joh/swc-array-config/build/lib/swc/.swcrc-amd / https://github.com/microsoft/vscode/blob/joh/swc-array-config/build/lib/swc/.swcrc-no-mod) it also aims similar.

So at least I'd say this can confuse some amount of users.

kwonoj avatar Sep 03 '22 05:09 kwonoj

This is because of some other tools actually behave in multiple-emission way even though it's not 100% same. For example, https://webpack.js.org/configuration/configuration-types/#exporting-multiple-configurations webpack emits multiple bundles based on multiple configuration inputs. (again, not 100% identical, but)

kwonoj avatar Sep 03 '22 05:09 kwonoj

My reasoning about the guess is that there's no way to specify name of output files, so the user should not expect it to emit multiple files. After all, it should emit output to same file. All build tools that behave in that way provide a way to specify the output file name.

But well, I think this feature was a mistake. Maybe we need a more general dynamic configuration API like starlark

kdy1 avatar Sep 03 '22 05:09 kdy1

Yes, my general impression so far is this is not a well-defined interface at least.

I'd like to approach this in 2 perspective, first would like to confirm original issue's intent and provide possible workaround. And also attempt to remove this feature, prepare better way to support specific usecases.

kwonoj avatar Sep 03 '22 05:09 kwonoj

Or simply, we take this back and enforce .swcrc to not allow arrays. It's simple, but breaking changes, and possibly disappoint @jrieken 😅 .

That's totally fine for me.

Our challenge is the following: One file (might be more in the future) cannot be emitted with the AMD-stuff around it, because it is AMD bootstrap code. E.g it should be emitted as-is. That's why I originally filed https://github.com/swc-project/swc/issues/4989. I couldn't get this to work with a single config and therefore created two ones. Maybe I misunderstood https://github.com/swc-project/swc/issues/4989#issuecomment-1159346961 and you can help to configure things in a single config file?

jrieken avatar Sep 05 '22 14:09 jrieken

Is using bindings api (transform*) is no-go? it sounds like most straightforward answer to the requirement. Otherwise, probably invoking cli multiple times per each different config would be a way to go. It's nothing wrong, while it isn't the most elegant way to do it though. SWC currently doesn't support any way to set configuration values dynamically in .swcrc unfortunately, which is the root cause of all this conversations in my opinion.

kwonoj avatar Sep 06 '22 04:09 kwonoj

Is using bindings api (transform*) is no-go? it sounds like most straightforward answer to the requirement.

I had tried that in the past and for some reason didn't continue with it. I gave this another shot and I am happy to report success 🚀 . We now have SwcTranspiler which can be used interchangeably with our TscTranspiler. We keep both variants to reduce the "bus factor" and to be able to adopt newer TS versions faster than you.

A full transpile-run using SWC (VS Code and its built-in extension) takes 12s on our CI/CD machines. That is very awesome and improves our daily dev workflow significantly.

Thanks

btw - I don't mind if you close this issue without code changes

jrieken avatar Sep 07 '22 09:09 jrieken

A full transpile-run using SWC (VS Code and its built-in extension) takes 12s on our CI/CD machines. That is very awesome and improves our daily dev workflow significantly.

Great to hear! Please let us know if there are something we can followup later.

btw - I don't mind if you close this issue without code changes

Thanks for the confirmation. I think issue itself is still legit, even though we may not change things immediately we'll keep this for a while.

kwonoj avatar Sep 07 '22 16:09 kwonoj

@jrieken Did you use SwcTranspiler to transpile both .tax and .jsx files individually

samrat41 avatar May 18 '23 22:05 samrat41