Skip to content
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

Merged
merged 24 commits into from Jun 13, 2021
Merged

feat: rewrite in ESModules #407

merged 24 commits into from Jun 13, 2021

Conversation

wolfy1339
Copy link
Member

No description provided.

@wolfy1339 wolfy1339 added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Apr 22, 2021
@ghost ghost added this to Maintenance in JS Apr 22, 2021
@wolfy1339 wolfy1339 linked an issue Apr 23, 2021 that may be closed by this pull request
wolfy1339 added a commit that referenced this pull request May 4, 2021
Required for #407 as `tap` doesn't seem to like ESModules
@wolfy1339 wolfy1339 force-pushed the esm-rewrite branch 4 times, most recently from 818dcf1 to 88a2f4b Compare May 5, 2021 03:19
@wolfy1339
Copy link
Member Author

There's 3 commits that aren't totally related to this PR, that fix deprecation warnings with tap

  • fix: replace deprecated t.is with t.equal 2ce8b05
  • fix: replace t.isA with t.type c5bf865
  • fix: replace t.deepEqual with t.same 9a5cbbf

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.

@wolfy1339 wolfy1339 requested a review from gr2m May 6, 2021 02:14
@gr2m gr2m self-assigned this May 7, 2021
@ghost ghost moved this from Maintenance to In progress in JS May 7, 2021
@gr2m
Copy link
Contributor

gr2m commented May 7, 2021

you had incorrect imports. If the file has

export default temporaryRepository;
export const regex

then you cannot access temporaryRepository.regex, you have to do

import temporaryRepository, { regex } from '...'

@gr2m gr2m changed the title 🚧 feat: rewrite in ESModules feat: rewrite in ESModules May 7, 2021
@gr2m gr2m changed the title feat: rewrite in ESModules 🚧 feat: rewrite in ESModules May 7, 2021
@gr2m
Copy link
Contributor

gr2m commented May 7, 2021

Hm I cannot reproduce the failing test locally, can you?

Also npm run record is currently failing, I'm looking into that

@wolfy1339
Copy link
Member Author

I cannot reproduce the failing test locally with NodeJS v16

@wolfy1339
Copy link
Member Author

Also npm run record is currently failing, I'm looking into that

It's because in ESM you can't implicitly import the index.js located in a folder, you have to import it explicitly. Which I did account for. Must've missed these ones
I found another case of it in bin/remove-temporary-repositories.js

@gr2m gr2m removed their assignment May 7, 2021
@ghost ghost moved this from In progress to Maintenance in JS May 7, 2021
gr2m pushed a commit that referenced this pull request May 28, 2021
Required for #407 as `tap` doesn't seem to like ESModules
@gr2m
Copy link
Contributor

gr2m commented May 28, 2021

depends on #411

wolfy1339 added a commit that referenced this pull request May 28, 2021
Required for #407 as `tap` doesn't seem to like ESModules
gr2m pushed a commit that referenced this pull request Jun 11, 2021
Required for #407 as `tap` doesn't seem to like ESModules
@gr2m gr2m added the esm label Jun 11, 2021
@wolfy1339
Copy link
Member Author

Alright, only 3 failing tests remain.

There's a problem with bin/record.js --update, it's removing data from fixtures.

@gr2m gr2m self-assigned this Jun 13, 2021
@ghost ghost moved this from Maintenance to In progress in JS Jun 13, 2021
@gr2m
Copy link
Contributor

gr2m commented Jun 13, 2021

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

@gr2m gr2m removed their assignment Jun 13, 2021
@ghost ghost moved this from In progress to Maintenance in JS Jun 13, 2021
@wolfy1339
Copy link
Member Author

Pinging @octokit/js-community
Can of you look at this to help figure out why the IDs are differing between the CommonJS version and this PR?

@wolfy1339
Copy link
Member Author

The problem seems to stem from using Promises.
Making the test use a for-of loop instead of Promise.all makes it pass. (also changing the Promise.all in lib/normalize/index.js)

@wolfy1339
Copy link
Member Author

wolfy1339 commented Jun 13, 2021

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

@gr2m
Copy link
Contributor

gr2m commented Jun 13, 2021

sounds great 👍🏼 thanks @io4! Can you push the change?

@wolfy1339 wolfy1339 changed the title 🚧 feat: rewrite in ESModules feat: rewrite in ESModules Jun 13, 2021
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Great work 👏🏼

@wolfy1339 wolfy1339 merged commit b3a45cf into master Jun 13, 2021
@wolfy1339 wolfy1339 deleted the esm-rewrite branch June 13, 2021 22:02
JS automation moved this from Maintenance to Done Jun 13, 2021
@github-actions
Copy link
Contributor

🎉 This PR is included in version 22.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
No open projects
JS
  
Done
Development

Successfully merging this pull request may close these issues.

Rewrite as native ES Module
2 participants