Skip to content

Move node-cli to be pure ESM, use Conf for persistent storage#203

Closed
hl662 wants to merge 35 commits intomainfrom
nam/esm-conf
Closed

Move node-cli to be pure ESM, use Conf for persistent storage#203
hl662 wants to merge 35 commits intomainfrom
nam/esm-conf

Conversation

@hl662
Copy link
Copy Markdown
Contributor

@hl662 hl662 commented Aug 23, 2023

  • Modify Node-cli's tsconfig.json and package.json to emit ESM on build
  • Replace node-persist with Conf, very similar to electron-store used by @itwin/electron-authorization
  • Addresses Move to ESM #166

hl662 and others added 30 commits August 18, 2023 17:03
Co-authored-by: Arun George <11051042+aruniverse@users.noreply.github.com>
Co-authored-by: Arun George <11051042+aruniverse@users.noreply.github.com>
} from "@openid/appauth";
import { NodeCrypto, NodeRequestor } from "@openid/appauth/built/node_support";
import { TokenStore } from "./TokenStore";
import { NodeCrypto, NodeRequestor } from "@openid/appauth/built/node_support/index.js";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need to modify imports of our deps to point to the js file?

Copy link
Copy Markdown
Contributor Author

@hl662 hl662 Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately, yes...
Relative imports need to explicitly have the file extensions .js attached to them when we set module and moduleresolution to Node16 or NodeNext. That was a rule seemingly defined by the TS team - what the TS compiler does is try to resolve the import to a specific file. If it can't find a specific file, it adds a .js extension (this is what the node resolver does). Apparently, this is a side-effect, rather than the compiler's intention.

Specifying --experimental-specifier-resolution=node would have worked, bypassing the TS linter rules, but it's experimental, and starting from Node 19 this flag was dropped :(

See the comments on this post on an explanation on what goes on underneath the compiler, and see this github discussion on the TS devs being tired of explaining their reasoning 😢

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you can replicate the effects from this flag --experimental-specifier-resolution=node by creating a custom loader for node but yeah that sounds much more troublesome than just adding .js extension 😁

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading all the comments about this, it's starting to make sense... including the explicit file extension improves module resolution performance (the loader doesn't need to go through every permutation of .js or .d.js or .ejs), and it reduces ambiguity. The thing is, not having the file extension was how the majority of devs have done it in the past, so while including it is the recommended, it's doesn't help with uniformity

@hl662 hl662 closed this Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants