Bump npm deps

[lnicola]

No description provided.

[matklad]

Bloody typical :-)

[lnicola]

@Veetaha do you have any idea why the extension doesn't start after upgrading from @rollup/plugin-commonjs 12 to 13?

[Veetaha]

@lnicola what is the error?

[lnicola]

No error, it just doesn't load.

[Veetaha]

How to reproduce this?

[lnicola]

Bump @rollup/plugin-commonjs to ^13.0.0 in package.json.

[Veetaha]

And then how do you check that it works?

[lnicola]

cargo xtask install --client-code and open a project in Code.

[Veetaha]

@lnicola Is there anything in the developer tools console?

[lnicola]

No, just the usual error about the proposed APIs.

[Veetaha]

Module and bundling story in JavaScript (and even worse - TypeScript over it) is genuinely one of the worst things I've ever had to work with. I can spend hours figuring out what's wrong.

Some time ago there was a warning about mixing named and default exports.
I couldn't understand why it was emitted, thus I created an issue in rollup repo: rollup/rollup#3368
No activity there so far and I predict there will be none further too.
So we just disabled this warning (#3158) according to the suggestion that was included in that warning message body:

Use `output.exports: 'named'` to disable this warning

And we lived happily thereafter.
Until this PR rollup/plugins#427 probably has brought us a breaking change.

Now removing exports: 'named' entry is what fixes the breaking change (which is ridiculous, because this was our workaround and this was suggested by the warning message).
https://github.com/rust-analyzer/rust-analyzer/blob/513924a7e01ef81a03869249c902daf148439736/editors/code/rollup.config.js#L8-L22

@lnicola long story short I cannot easily explain whether this was a bug in rollup or its commonjs plugin, but removing exports: 'named' or setting exports: 'default' should let us upgrade to @rollup/[email protected].

I'll dare to have a luck to ask @lukastaegert to elaborate on why there was such a warning in the first place and why v13 has brought a breaking change to us.

Small intro to our case: all we have at the top level is a simple vscode extension, i.e. a file with two exported functions:

export async function activate(context: vscode.ExtensionContext) { }
export async function deactivate() { }

[lnicola]

Oof, thanks for figuring that out.

[lukastaegert]

There may be bugs in the latest version of the CJS plugin that explain the change in behaviour. We are currently reviewing how interop is handled here. Nevertheless going back to the original issue, as your entry point only has named exports and without looking into your setup too deeply I assume you transpile everything to CJS via tsc first before you run Rollup? This would explain the mixed exports as CJS files always have an implicit default export at the moment. I cannot really recommend this: This can unnecessarily bloat your code with unneeded interop and make other optimizations more difficult for Rollup. If it is possible, you should try to switch to ESM output. Or use @rollup/plugin-typescript which will tweak the settings for you. This will get rid of the mixed exports that were only a double transpilation artefact. But these are just some thoughts.

[Veetaha]

Yes, we use tsc to transpile .ts to CJS, but the transpiled code is just

exports.activate = activate;
exports.deactivate = deactivate;

I don't know what this implicit default export for CJS means and what it is useful for, because we clearly don't do neither module.exports = foo nor module.exports.default = foo, I suppose this is something that rollup desugars CJS file to (i.e. file with phantom "default" export (I guess ?)).

Thanks for the suggestions, unfortunately, we cannot easily use tsc to transpile stuff to ESM or use the @rollup/plugin-typescript for the sake of easier time with debugging our extension. However, we do should turn off the interop options in tsconfig.json since they just add some amount of code bloat:
https://github.com/rust-analyzer/rust-analyzer/blob/513924a7e01ef81a03869249c902daf148439736/editors/code/tsconfig.json#L9-L10

[lukastaegert]

module.exports itself is the default export. In CJS it exists, even though you do not assign stuff to it. We hope to improve this for transpiled ES modules eventually, though.

[Veetaha]

Aha, okay, got it, thanks for the explanation!

[matklad]

bors d=Veetaha

[matklad]

bors d=Veetaha

[bors]

✌️ Veetaha can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

[lnicola]

I'd propose to make it 12.12.x (there is no @types/node 12.13 version published yet)

I picked 12.8.1 as that's what Code uses, at least in 1.46.1 (although we still support 1.45). 12.12 seems more arbitrary, even if they map to the same version because of semver.

EDIT: whatever, newer is newer.

[Veetaha]

bors r+

[bors]

Build succeeded:

• Rust (ubuntu-latest)
• TypeScript (windows-latest)
• Rust (windows-latest)
• TypeScript (macos-latest)
• Rust (macos-latest)
• TypeScript (ubuntu-latest)

[lnicola]

However, we do should turn off the interop options in tsconfig.json

We can turn off allowSyntheticDefaultImports, but not esModuleInterop:

tests/unit/index.ts:2:8 - error TS1259: Module '"mocha"' can only be default-imported using the 'esModuleInterop' flag

2 import Mocha from 'mocha';
         ~~~~~

  node_modules/@types/mocha/index.d.ts:2910:5
    2910     export = Mocha;
             ~~~~~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

tests/unit/index.ts:3:8 - error TS1259: Module '"node_modules/@types/glob/index"' can only be default-imported using the 'esModuleInterop' flag

3 import glob from 'glob';
         ~~~~

  node_modules/@types/glob/index.d.ts:88:1
    88 export = G;
       ~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

Unless we can import those differently (what's a default import?).

[Veetaha]

Default import is:

import AliasName from "module";

Namespace import is

import * as AliasName from "module";

Namespace import has only the single form, but default import can be combined with destructuring imports:

import AliasName, { SymbolName1, SymbolName2 } from "module";

Default import only refers to the value exported with

export default <value here>;

I really don't understand why we need this complexity at all. Default import/export was created to allow for exporting a single value from the module with shorter syntax (no curly braces, no * as as in NamescapeImport).

If I am not mistaken you cannot use the alias created by NamespaceImport as a function according to ECMAScript specs (but this requirement is relaxed in TypeScript I guess?).
This also complicates due to the interop with commonjs modules style (i.e. require(module)), there is even a special syntax for them in TypeScript

import AlaisName = require('module');

But with esModuleInterop and allowSyntheticDefaultImports you can use default import instead of import _ = require("").

I myself don't know all the nitty-gritty of this stuff, I remember testing the following and it worked:

- import Mocha from 'mocha';
+ import * as Mocha from 'mocha';
- import * as glob from 'glob';
+ import * as glob from 'glob';

[Veetaha]

My recommendation is to never use default import/export and always use namespace import or destructuring