-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move node-cli to be pure ESM, use Conf for persistent storage #203
Open
hl662
wants to merge
35
commits into
main
Choose a base branch
from
nam/esm-conf
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+166
−88
Open
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
67e1402
Migrate from node-keytar to crypto
hl662 3a739b4
add option to configure refresh token storage path
hl662 c3577ce
move @types/node-persist to devDep
hl662 d3f9ea9
update pnpm-lock
hl662 f29e56b
rush change
hl662 81edc40
Apply suggestions from code review
hl662 3690ccb
delete cached entry when scopes are different
hl662 4d3339a
Merge branch 'nam/node-cli-keytar' of github.com:iTwin/auth-clients i…
hl662 13d7cde
resolve merge conflicts
hl662 1f1f120
Merge branch 'main' into nam/node-cli-keytar
hl662 a98f439
Change files
hl662 b3114a9
remove previous rush change file
hl662 2e462ee
move from cjs to esm
hl662 f9f9f6d
Merge branch 'nam/node-cli-keytar' into nam/esm-conf
hl662 7dce78b
use Conf in node-cli esm
hl662 17f9e6c
add main field in package.json, fix config path for token store
hl662 da4ba81
move from cjs to esm
hl662 77aba02
Migrate from node-keytar to crypto
hl662 35dd5ff
add option to configure refresh token storage path
hl662 8a7aae6
move @types/node-persist to devDep
hl662 2519feb
rush change
hl662 fdefb4d
delete cached entry when scopes are different
hl662 3d2c41e
Apply suggestions from code review
hl662 0cba13f
resolve merge conflicts
hl662 45a1ac1
remove previous rush change file
hl662 04640d5
use Conf in node-cli esm
hl662 aff3dae
add main field in package.json, fix config path for token store
hl662 5289bb3
Merge branch 'nam/esm-conf' of github.com:iTwin/auth-clients into nam…
hl662 e531e3e
add interface definition for object stored
hl662 e6e1c4f
Change files
hl662 f6b9dc4
revert mocha config spec change
hl662 aadfada
remove mix of import and require in statements
hl662 5bcda63
use nodenext in tsconfig, use 4.x in core deps
hl662 c33fc77
bump itwin build tools off of dev
hl662 0d2c008
fix indent
hl662 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
change/@itwin-node-cli-authorization-cd9b1e0d-dbd2-4a0a-8305-caef61275076.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "major", | ||
"comment": "Move from cjs to esm and use Conf", | ||
"packageName": "@itwin/node-cli-authorization", | ||
"email": "50554904+hl662@users.noreply.github.com", | ||
"dependentChangeType": "patch" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 toNode16
orNodeNext
. 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 thenode
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.
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 😁
There was a problem hiding this comment.
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