-
Notifications
You must be signed in to change notification settings - Fork 139
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
update got dependency and convert to esm module #533
Conversation
523a969
to
4bafe0b
Compare
@@ -8,7 +8,6 @@ Relates OR Closes #0000 | |||
|
|||
### Checklist | |||
- [ ] Added [CHANGELOG](https://github.com/hashicorp/vault-action/blob/master/CHANGELOG.md) entry (only for user-facing changes) | |||
- [ ] Did not commit changes to `dist/index.js` (This is only done for releases by vault-action maintainers) |
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.
We actually do need to commit the distribution files so that each PR's e2e tests run against the changes.
@@ -34,18 +35,15 @@ | |||
}, | |||
"homepage": "https://github.com/hashicorp/vault-action#readme", | |||
"dependencies": { | |||
"got": "^11.8.6", | |||
"@actions/core": "^1.10.1", | |||
"got": "^14.2.1", |
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.
This dep update is the reason for this monster PR. 😶🌫️
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.
Thank you for your effort in this dependency update! 🥇
This looks good from my initial pass. I have one comment.
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.
LGTM. Thank you!
Looks like this change has broken the action as can be seen in this comment. Unfortunately, our E2E tests and local tests don't reveal this issue. It has something to do with the fact that they use the local path:
But I am not sure exactly why that succeeds and the following fails:
However, there is good news. (1) This change wasn't released yet and (2) It looks like there is experimental support for require()-ing synchronous ES modules in Node.js from nodejs/node#51977 which should be available in the next major node Version (v22?). That should allow us to revert this PR and import |
This reverts commit 77efb36.
Description
This PR proposes the following changes:
Also, apparently ncc can't compile the
jsonata
dependency to ESM properly so we add a custom require to be able to import it. I may be doing something wrong here but this is what I came up with based on vercel/ncc#791Why?
vault-action’s got v11 dependency is no longer maintained. We need to either convert to ESM or use the dynamic import() function to upgrade to v14. This PR takes the former route.
RE: jest -- The jest testing framework does not have good support for ESM modules. It requires enabling experimental features and doesn't have good support for mocking. So we swap to vitest which has full ESM support. See https://stackoverflow.com/questions/68956636/how-to-use-esm-tests-with-jest and https://jestjs.io/docs/ecmascript-modules
See #516
Alternatives
We could look into switching to another HTTP library so that we don't have to convert our project to an ESM module. However, I opted to stick with got so that we don't have to change any of the HTTP configuration and handling.
Checklist
Community Note
reaction
to the original pull request comment to help the community and maintainers
prioritize this request
followers and do not help prioritize the request