feat: Add editor SDK support for oxc (oxfmt & oxlint)#7078
feat: Add editor SDK support for oxc (oxfmt & oxlint)#7078slainless wants to merge 12 commits intoyarnpkg:masterfrom
Conversation
| // We are using pass-through here since we don't really need to change the default behavior of the wrapper. | ||
| // Since the oxlint wrapper is the one that spawns the actual oxlint binary, we extend the PATH here | ||
| // to enable the tsgolint PATH resolution strategy in the next tsgolint wrapper. | ||
| const oxlintMonkeyPatch = ` | ||
| module => module; | ||
|
|
||
| process.env.PATH += \`;\${resolve(__dirname, '../../oxlint-tsgolint/bin')}\`; | ||
| import(\`${ppath.join(npath.toPortablePath(relPath), `dist/cli.js`)}\`); | ||
| `; |
There was a problem hiding this comment.
- I think
oxlintnot exportingbin/oxlintor evenpackage.jsonis a problem that should be fixed upstream. - Unfortunately, we cannot fully support ESM SDKs until
import.meta.resolve(specifier, parent)is unflagged. But for now I think a stopgap similar to this is acceptable
There was a problem hiding this comment.
Looks like the import is still necessary even if we change the exports setup and the entrypoint back to bin/oxlint. The top-level await is the blocker :/
There was a problem hiding this comment.
With proper exports from oxlint we can use require.resolve to get the package directory instead of needing to go through the PnP API and/or to read oxlint/package.json to get the correct bin path instead of hardcoding it.
The top-level await is the blocker
The more proper fix would be to actually support generating ESM SDK files, but to do so we need import.meta.resolve(specifier, parent) as a replacement for createRequire.
Short of that, this stopgap is fine for me for the time being like I said.
There was a problem hiding this comment.
With proper exports from
oxlintwe can userequire.resolveto get the package directory instead of needing to go through the PnP API and/or to readoxlint/package.jsonto get the correct bin path instead of hardcoding it.
Ah, I understand now, thanks! I haven't though that far. It does more natural to resolve via the created require, yeah. I will try to notify upstream package.
|
Is adding --loader/--experimental-loader flag to However, I don't know the impact of this if applied alongside the setupEnv flag without its own dedicated option flag. I assume it is ok since the default runner inject both --require and --experimental-loader by default? |
| const wrapper = new Wrapper(`oxfmt` as PortablePath, {pnpApi, target}); | ||
|
|
||
| await wrapper.writeDefaults(); | ||
| await wrapper.writeFile(`bin/oxfmt` as PortablePath, { |
There was a problem hiding this comment.
I missed this the first time around, but binary files should use writeBinary to set the executable flag. Similarly for the other 2 wrappers
There was a problem hiding this comment.
Thanks, it should be fixed now
I feel like we should have done that already, but just never needed to so nobody did.
Looks like currently nothing else uses the No developer likes hackiness, but I think this is as far as we can get. So it's fine by me if it does solve a blocker for some people. |
2 participants
What's the problem this PR addresses?
Add editor SDK support for oxc. This should enable optional editor integration with both oxfmt and oxlint.
How did you fix it?
Added oxlint, oxfmt and oxlint-tsgolint as supported integration and documented it.
Oxlint and oxlint-tsgolint addition is a bit hacky and I don't know whether this is the right direction or not. Rationale can be read from the comment there.
Loader option added to sdk setupEnv to prevent oxfmt tinypool from being broken due to failing to resolve tinypool/dist/process.js.
I haven't tried this with vim yet since I don't use it so if anyone would kindly help test whether this will work on vim or not, thanks!
oxlint type-aware feature (tsgolint)
The biggest challenge here is the type-aware feature (tsgolint) support. The rust side of oxlint spawn tsgolint a bit different from how the oxlint itself is spawned:
https://github.com/oxc-project/oxc/blob/d3dcf5bc9718ebb4839be27062b5d82da2118e2e/crates/oxc_linter/src/tsgolint.rs#L265-L270
Instead of using js entrypoint, it directly spawn executable so the wrapper approach is not directly applicable here. Luckily, there are some strategies that the oxc_linter uses to resolve tsgolint:
The only realistic strategy is to use PATH. For the sdks, it will generate a windows executable shim (cmd) that will basically just spawn tsgolint.js, else should fallback to tsgolint.
Be noted however that this will not fix fundamental issue with typescript-go module resolution, which requires this to be merged first:
This PR just enable the usage of tsgo regardless of whether it can resolve with pnp or not in the type-checking.
Reference
Coc config:
https://github.com/oxc-project/coc-oxc/blob/7a7f0d8f503d5cdb65da37232da6b865f159b42a/src/common.ts#L27-L36
Vscode oxc binary resolution reference:
https://github.com/oxc-project/oxc-vscode/blob/main/client/findBinary.ts
Oxlint-tsgolint executable resolution:
https://github.com/oxc-project/oxc/blob/d3dcf5bc9718ebb4839be27062b5d82da2118e2e/crates/oxc_linter/src/tsgolint.rs#L1164-L1225
Checklist