Understand why ably/modules is causing an "unexpected module syntax" error in npx attw

[lawrence-forooghian]

For now I've disabled this check, but would be good to understand. One way to fix it is to change the exports."./modules".default in package.json to exports."./modules".browser (i.e. to not advertise this package to Node), but according to this documentation we should have a default field, for future browser-like environments.

┆Issue is synchronized with this Jira Bug by Unito

[sync-by-unito]

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3994

[VeskeR]

The error comes from Node.js not being able to correctly resolve our ably/modules as an ES module, as it thinks it's a CommonJS module and imports it incorrectly.
This is due to in Node.js files are determined to be ES or CommonJS modules by their file extension (cjs for CommonJS and mjs for ESM) and ancestor package.json "type" field.
In our case, we don't set the type: "module" property in our package.json (which we shouldn't, as most of our exports are CommonJS modules), and we build build/modules/index.js with a .js extension, causing Node.js to resolve that the ably/modules is a CommonJS module, which leads to "Unexpected module syntax" error when checking via attw.

The fully correct solution would be to change output file extension for build/modules/index.js to .mjs, modules.d.ts to modules.d.mts (same rules apply to declaration files) and make modules.d.mts to be auto-generated based on source files using tsc/esbuild with the same settings as we build build/modules/index.js. We would also need to change exports."./modules" in package.json to look like this:

  "exports": {
    "./modules": {
      "types": "./modules.d.mts",
      "module": "./build/modules/index.mjs"
      "import": "./build/modules/index.mjs"
    }
  }

Which would indicate that ably/modules is purely an ES module and can only be used in environments that support ESM module resolution.

The main issue with this solution is changing generation rules for modules.d.ts. This type declaration file is currently hand-written, and it might be challenging to switch to auto-generation and make everything correct. Since modular variant of the library is not intended to work in Node.js anyway, these changes seem unnecessary right now.

---

A better solution for now, rather than completely ignoring the "Unexpected module syntax" error (which is quite vague), might be the following:
change file extension for build/modules/index.js to .mjs since it's an ESM, and change exports."./modules" in package.json to:

  "exports": {
    "./modules": {
      "types": "./modules.d.ts",
      "module": "./build/modules/index.mjs"
      "import": "./build/modules/index.mjs"
    }
  },

This way we avoid tinkering with modules.d.ts for now.
Using module and import conditions for exports field is also less restrictive than using browser as we indicate that any environment that supports ESM module resolution can import ably/modules.
This will still cause "Masquerading as CJS" error for node16 (from ESM) module resolution mode, which is due to modules.d.ts file being considered as a CommonJS file by Node.js. It is a less broad error than "Unexpected module syntax", which could be fully fixed if we switch to auto-generating type declaration files in the future.

@lawrence-forooghian let me know what you think

[lawrence-forooghian]

I'm not sure I understand why switching the extension of modules.d.ts to modules.d.mts means having to switch to using auto-generated declarations; would you mind explaining please?

Also, what does the module condition achieve?

[VeskeR]

Also, what does the module condition achieve?

Took one more look, it seems like the docs use it only when "require" condition is also present. Based on this explanation, it does seem to provide some bundle step optimization for Node.js, if those bundlers can resolve ES modules via require().

Since we don't provide CommonJS module for ably/modules and are not interested in Node.js in consuming this module, I think we can skip "module" condition since it's optional anyway.

why switching the extension of modules.d.ts to modules.d.mts means having to switch to using auto-generated declarations

Switching to modules.d.mts results in attw spitting out a bunch of errors:

Most notable Internal resolution error. Common causes include Declarations and JavaScript are generated separately, which is the case for us. We compile TS files and bundle them using esbuild, but type definitions are hand-written in modules.d.ts. Node.js can't figure it out, so causes an error on import.
Frankly speaking, when using modules.d.ts (without mts), and we get Masquerading as CJS error, this is still the same root issue, just less severe, because at least Node.js managed to figure it out and import the package.

To solve this, declaration files would need to be auto-generated using tsc (esbuild doesn't support outputting declaration files), but then we run into issues of combining declaration files into one (tsc doesn't allow it for ESM) and still doesn't guarantee that issue would be resolved, since js would be generated by esbuild, and .d.mts files by tsc.

[lawrence-forooghian]

So, if I've understood correctly, in order to get attw to not emit any error, then we must rename modules.d.ts to modules.d.mts?

I still don't understand, however, why doing this means that we must switch to using auto-generated declaration files. The "internal resolution error" page describes a bunch of reasons why this error might occur. My reading of that page is that, if you use hand-written declaration files and those files, for some reason, don't match (the exact kinds of mismatch that might trigger the error not clear to me) what's in the JavaScript bundle, then you will get this error. So, perhaps it's worth spending some time to try and understand what is the mismatch that’s causing this error, and maybe we can just fix that?

(By the way, I fully agree that being able to switch to automatically generating the declaration files would be a good thing. The main challenge there — in addition to what you mentioned — is making sure that doing so wouldn't change our shipped type declarations in a way that changes the declared public API of the SDK. Because, currently, our public API is what's contained in the type declaration files, not the types contained in the implementation. And these types don't always match — they should, but they don’t. So we'd need to go through a process of aligning them first.)

[VeskeR]

So, if I've understood correctly, in order to get attw to not emit any error, then we must rename modules.d.ts to modules.d.mts?

Yes, and in this case, we would need to fix the "Internal resolution error" emitted by attw due to the mismatch between declaration/js files. See below.

I still don't understand, however, why doing this means that we must switch to using auto-generated declaration files.

if you use hand-written declaration files and those files, for some reason, don't match ... what's in the JavaScript bundle, then you will get this error

This is exactly it. attw goes over js files, analyzes their exports, and compares them to declaration files. If there is a mismatch, depending on other conditions, we will get one of attw's errors. In our case, we get the "Internal resolution error".

(the exact kinds of mismatch that might trigger the error not clear to me)

And here is a troubled part.
I also couldn't find any explanation on what a "mismatch" actually is. There are no helpful infromation from the lib, like on what lines of code it got an error, we're stuck with this generic "Internal resolution error" error to deal with, and I have no idea what's the actual reason behind it.
What I did find out though, is that not declaring something that exists in the code - is not a "mismatch". We do it for our regular ably typings - we skip some things that we consider "internal", but which are public in the code - and we don't get an error from attw.

Also "mismatch" can be a really convoluted issue. There was an issue I fixed in v2 branch with imports in react hooks #1619 - we were imporing typings from ably using ../../../../../ably.js string in react hooks code, instead of ably. This type of import caused problems in pnpm specifically #1495 - it couldn't resolve it, and it was fixed on main branch (and was lost on v2 branch at some point). But everything worked on npm without any issues, that's why we didn't catch the problem with that import locally.
That fix on main was part of the 1.2.48 release. Now, what's interesting, is that if you run attw check on the 1.2.47 release before the fix (we didn't have attw at that moment), it will actually show "Internal resolution error" errors for ably/react export for all resolution types. And the error goes away once you fix that incrrect import.
So attw somehow was able to detect failed typings import, even though it worked just fine in all runtimes using npm.

So, perhaps it's worth spending some time to try and understand what is the mismatch that’s causing this error, and maybe we can just fix that?

Now the problem with that, is that auto generated type declaration files for ESM are not bundlable. You cannot concatenate them in a single file, it is just not supported for ESM specifically. So even that by itself can be a reason why attw if throwing errors - it expects a separate type declaration file for each source js module/file - and we don't have that.
Or maybe this is not an issue, but something else. It would require an unknown amout of effort to figure the difference between hand-written file and auto generated ones, and I don't think it's justifiable in this case, since at the end of the day - this is an error for node resolution for ably/modular. We don't provide modular variant for the node, it can only work in browsers.

So, by simply changing index.js to index.mjs and default to import in package.json we will narrow the attw error to "Masquerading as CJS", which is better than what we have now. And what is more importantly - it would actually allow users to import from ably/modular in node environment and see the message that says that this lib variant is designed for browsers (currently we can't even do that, we get SyntaxError: Named export 'BaseRealtime' not found. The requested module 'ably/m odular' is a CommonJS module, which may not support all module.exports as named exports. error in node).

[lawrence-forooghian]

Thanks for the explanation. Let's go with your suggestion then.

[VeskeR]

Fixed in #1696