-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: rewrite in ESModules #407
Conversation
Required for #407 as `tap` doesn't seem to like ESModules
818dcf1
to
88a2f4b
Compare
There's 3 commits that aren't totally related to this PR, that fix deprecation warnings with
Should I split them off? Otherwise, the tests run, but they fail due to mismatches in expected output. I am unsure why that would happen with the switch to ESM. |
you had incorrect imports. If the file has export default temporaryRepository;
export const regex then you cannot access import temporaryRepository, { regex } from '...' |
Hm I cannot reproduce the failing test locally, can you? Also |
I cannot reproduce the failing test locally with NodeJS v16 |
It's because in ESM you can't implicitly import the |
Required for #407 as `tap` doesn't seem to like ESModules
depends on #411 |
Required for #407 as `tap` doesn't seem to like ESModules
Required for #407 as `tap` doesn't seem to like ESModules
First pass at rewriting in ESModules
Alright, only 3 failing tests remain. There's a problem with |
We are down the the normalize test failing because of how the entity IDs are calculated. I tried to figure out where the diff is coming from but I don't know. I think it might be that the keys in an object are sorted differently, because IDs from the recorded fixtures are mapped to the made-up IDs starting at 1000 instead. Maybe you have an idea what might be going on? It's odd because the record tests all pass, but when running the normalization test in isolation it finds a diff |
Pinging @octokit/js-community |
The problem seems to stem from using |
With this patch the test works normally. diff --git a/lib/normalize/index.js b/lib/normalize/index.js
index 129e5f8..a7c879c 100644
--- a/lib/normalize/index.js
+++ b/lib/normalize/index.js
@@ -83,17 +83,15 @@ async function normalize(scenarioState, fixture) {
const responses = Array.isArray(fixture.response)
? fixture.response
: [fixture.response];
- await Promise.all(
- responses.map(async (response) => {
- normalizeCommon(response);
- const entityName = toEntityName(response, fixture);
- if (entityName) {
- await (
- await import(`./${entityName}.js`)
- ).default(scenarioState, response, fixture);
- }
- })
- );
+ for (let response of responses) {
+ normalizeCommon(response);
+ const entityName = toEntityName(response, fixture);
+ if (entityName) {
+ await (
+ await import(`./${entityName}.js`)
+ ).default(scenarioState, response, fixture);
+ }
+ }
// update content length
if (/^application\/json/.test(fixture.headers["content-type"])) {
diff --git a/test/integration/normalize.test.js b/test/integration/normalize.test.js
index 9166649..accbe2f 100644
--- a/test/integration/normalize.test.js
+++ b/test/integration/normalize.test.js
@@ -19,9 +19,11 @@ glob
commitSha: {},
ids: {},
};
- const actual = await Promise.all(
- raw.filter(isntIgnored).map(normalize.bind(null, scenarioState))
- );
+ const actual = [];
+ for (let item of raw.filter(isntIgnored)) {
+ let result = await normalize.bind(null, scenarioState)(item);
+ actual.push(result);
+ }
expect(actual).toEqual(expected);
});
}); All thanks to my friend @io4 who diagnosed this and found the issue |
sounds great 👍🏼 thanks @io4! Can you push the change? |
Co-Authored-By: Lucas Fiegl <[email protected]>
…ts for jest tests
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.
Great work 👏🏼
🎉 This PR is included in version 22.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.