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

Config files fail with top-level array #5720

Open
jrieken opened this issue Sep 2, 2022 · 16 comments
Open

Config files fail with top-level array #5720

jrieken opened this issue Sep 2, 2022 · 16 comments
Labels
Milestone

Comments

@jrieken
Copy link

jrieken commented Sep 2, 2022

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
Copy link
Author

jrieken commented Sep 2, 2022

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

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

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.

@kdy1 kdy1 added this to the Planned milestone Sep 3, 2022
@kdy1
Copy link
Member

kdy1 commented Sep 3, 2022

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

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

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.

@kdy1
Copy link
Member

kdy1 commented Sep 3, 2022

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()

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

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.

@kdy1
Copy link
Member

kdy1 commented Sep 3, 2022

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)

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

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
Copy link
Member

kwonoj commented Sep 3, 2022

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)

@kdy1
Copy link
Member

kdy1 commented Sep 3, 2022

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

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

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.

@jrieken
Copy link
Author

jrieken commented Sep 5, 2022

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 #4989. I couldn't get this to work with a single config and therefore created two ones. Maybe I misunderstood #4989 (comment) and you can help to configure things in a single config file?

@kwonoj
Copy link
Member

kwonoj commented Sep 6, 2022

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.

@jrieken
Copy link
Author

jrieken commented Sep 7, 2022

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

@kwonoj
Copy link
Member

kwonoj commented Sep 7, 2022

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.

@samrat41
Copy link

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

@kdy1 kdy1 modified the milestones: Planned, Next major Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants