Move node-cli to be pure ESM, use Conf for persistent storage #203
Conversation
Co-authored-by: Arun George <11051042+aruniverse@users.noreply.github.com>
…nto nam/node-cli-keytar
Co-authored-by: Arun George <11051042+aruniverse@users.noreply.github.com>
hl662
requested review from
evelynpreslar-bentley,
aruniverse and
ben-polinsky
as code owners
| import { assert, BeEvent, BentleyError, Logger } from "@itwin/core-bentley"; | ||
| import { | ||
| AuthorizationError, AuthorizationNotifier, AuthorizationRequest, AuthorizationRequestHandler, AuthorizationResponse, | ||
| AuthorizationServiceConfiguration, BaseTokenRequestHandler, BasicQueryStringUtils, GRANT_TYPE_AUTHORIZATION_CODE, GRANT_TYPE_REFRESH_TOKEN, | ||
| TokenRequest, | ||
| } 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"; |
There was a problem hiding this comment.
do we really need to modify imports of our deps to point to the js file?
There was a problem hiding this comment.
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 😢
There was a problem hiding this comment.
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 😁
There was a problem hiding this comment.
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
tsconfig.jsonandpackage.jsonto emit ESM on buildnode-persistwith Conf, very similar toelectron-storeused by@itwin/electron-authorization