feat: esm and cjs exports by kanadgupta · Pull Request #427 · readmeio/fetch-har

kanadgupta · 2023-09-21T20:57:10Z

🧰 Changes

This PR refactors this codebase to support both ESM and CJS exports. I was originally blocked in #426 (comment) — turns out the issue was the treeshaking flag. Removing that (and separating out the type exports into types.ts) fixes the issue!

This PR also does a little bit of housekeeping:

Runs Prettier on everything
Bumps a few dependencies to the latest

🧬 QA & Testing

Before/after for Are The Types Wrong

If you check out this repo and run the following commands, they should work as expected:

# install + build
npm ci && npm run build

# run example scripts
node example.mjs
node example.cjs
deno run -A example.mjs

rafegoldberg

Gonna hold off approving till you mark this as "Ready for review", but generally this looks good to me. Left some comments/questions below.

rafegoldberg · 2023-09-26T19:04:25Z

@@ -1,4 +1,4 @@
-const fetchHAR = require('.').default;
+const fetchHAR = require('.');


Will this change to the export/import pattern break things for CJS users? Or will it also work from .default?

nah the default is no longer required — this is likely going to be a breaking change! the default thing was actually a bit of a code smell since we probably weren't exporting types properly

Worth calling out in the changelog (or in the commit message)?

yep will do in the GitHub release!

Co-Authored-By: Ilias Tsangaris <[email protected]>

that way we can use the conditional require syntax!

kanadgupta · 2023-09-26T21:21:34Z

-    "polyfills": [
-      "fetch",
-      "Headers",
-      "Request"
-    ]


these weren't necessary!

* chore(deps): bump @vitest/browser to latest * test: add MSW tests

(server) implies the existence of (client), which isn't a thing 😔

domharrington

LGTM!

domharrington · 2023-09-27T14:05:05Z

-    "undici": "^5.24.0"
+    "@readme/data-urls": "^3.0.0",
+    "@types/har-format": "^1.2.13",
+    "undici": "^5.25.1"


Do we need to bring this in if we're just using whatever native fetch() is present in the environment?

hmm we'll have to ask jon but i believe we still need it for node environments (see this comment and below):

fetch-har/src/index.ts

Lines 16 to 25 in 9e0360d

if (!globalThis.File) {

try {

// Node's native `fetch` implementation unfortunately does not make this API global so we need

// to pull it in if we don't have it.

// eslint-disable-next-line @typescript-eslint/no-var-requires

globalThis.File = require('undici').File;

} catch (e) {

throw new Error('The File API is required for this library. https://developer.mozilla.org/en-US/docs/Web/API/File');

}

}

@kanadgupta @domharrington We could move this to optionalDependencies but yeah this is only really needed for server environments.

Annoyingly, File only became a global in Node v20: https://nodejs.org/docs/latest/api/globals.html#class-file otherwise we could just use that.

Co-authored-by: Dom Harrington <[email protected]>

see here: https://www.totaltypescript.com/tsconfig-cheat-sheet#not-transpiling-with-typescript

... because i don't trust tsup lol

erunion · 2023-10-04T20:41:20Z

 import DatauriParser from 'datauri/parser';
 import express from 'express';
 import multer from 'multer';
+// @ts-expect-error this file is ESM


@kanadgupta Is this still needed?

oh whoops yeah as of #431 we don't need this! fixed in 0eea0ff

erunion

sick

kanadgupta added 8 commits September 20, 2023 11:07

feat: enable TS strict mode

4ef67c4

feat: exports for ESM and CJS

6fba5a5

fix: don't lint before tests

ff39a68

fix: try splitting out types from default export

59c6d89

chore: more tsconfig refreshes

fe7a714

fix: this was the goddamn issue!!!!

5126ea3

feat: migrate package to ESM, everything works 🥹

0854483

chore: lint fixes

a7f0958

kanadgupta added the enhancement New feature or request label Sep 21, 2023

kanadgupta changed the base branch from main to ts-strict-mode September 21, 2023 21:10

kanadgupta added 4 commits September 21, 2023 16:12

Merge branch 'ts-strict-mode' into esm-and-cjs

282d71d

chore(deps): remove unused dep

9273c6e

chore(deps): bump a bunch

d7e562b

ci: add pretest script

ea429af

kanadgupta mentioned this pull request Sep 21, 2023

chore(deps): bump @readme/data-urls from 1.0.1 to 2.0.0 #424

Closed

revert: don't lint in tests

b42332c

This was referenced Sep 22, 2023

fix(tsup): drop treeshaking flag, lint fixes readmeio/oas#798

Merged

feat: esm + tsup readmeio/api#734

Closed

feat: migrate api and core to ESM readmeio/api#733

Merged

kanadgupta added 2 commits September 26, 2023 12:01

chore: run prettier on everything

e9319d0

ci: small refactor

15de3f6

rafegoldberg reviewed Sep 26, 2023

View reviewed changes

kanadgupta and others added 7 commits September 26, 2023 14:08

fix: bump target down

775769a

Co-Authored-By: Ilias Tsangaris <[email protected]>

Merge branch 'ts-strict-mode' into esm-and-cjs

3b70e83

refactor: move type-checks into function itself

c3c85a1

revert: build this repo in CJS instead

8bd0f21

that way we can use the conditional require syntax!

chore: clean up eslint config

05125a7

chore: bump a few more deps

5b1e3e0

chore: bring back this rule

840b5ec

kanadgupta commented Sep 26, 2023

View reviewed changes

kanadgupta requested a review from rafegoldberg September 26, 2023 21:33

kanadgupta marked this pull request as ready for review September 26, 2023 21:33

kanadgupta requested review from gratcliff and ilias-t September 26, 2023 21:33

gratcliff approved these changes Sep 26, 2023

View reviewed changes

Comment thread package.json Outdated

kanadgupta and others added 6 commits September 26, 2023 16:54

chore: test type cleanup

07e458c

Merge branch 'ts-strict-mode' into esm-and-cjs

e75c68a

chore: silence TS error

e727190

fix: whoops remove the write flag

97e982d

test: add msw tests (#428)

eff8b6b

* chore(deps): bump @vitest/browser to latest * test: add MSW tests

chore: rename test

db953d1

(server) implies the existence of (client), which isn't a thing 😔

kanadgupta requested a review from domharrington September 27, 2023 13:44

refactor: remove readable-stream polyfill (#429)

46801c2

kanadgupta mentioned this pull request Sep 27, 2023

feat: enable TS strict mode #425

Merged

Base automatically changed from ts-strict-mode to main September 27, 2023 14:01

kanadgupta added 2 commits September 27, 2023 09:04

Merge branch 'main' into esm-and-cjs

1ee16ac

chore: add files array

0b9b354

domharrington approved these changes Sep 27, 2023

View reviewed changes

chore: ope clean up an unused type

2e73a92

rafegoldberg approved these changes Sep 27, 2023

View reviewed changes

kanadgupta and others added 4 commits September 27, 2023 09:07

Update README.md

9e0360d

Co-authored-by: Dom Harrington <[email protected]>

chore: add webpack thingy

6e88b70

fix: edit tsconfig.json

8f03d80

see here: https://www.totaltypescript.com/tsconfig-cheat-sheet#not-transpiling-with-typescript

chore: bring back prebuild script

c27d2e4

... because i don't trust tsup lol

kanadgupta merged commit bd9482a into main Sep 27, 2023

kanadgupta deleted the esm-and-cjs branch September 27, 2023 14:33

kanadgupta mentioned this pull request Sep 27, 2023

fix: convert library to module #431

Merged

erunion reviewed Oct 4, 2023

View reviewed changes

kanadgupta mentioned this pull request Jun 17, 2024

feat(sdk-snippets): library modernization with ESM+CJS builds readmeio/metrics-sdks#1017

Closed

		@@ -1,4 +1,4 @@
		const fetchHAR = require('.').default;
		const fetchHAR = require('.');

	if (!globalThis.File) {
	try {
	// Node's native `fetch` implementation unfortunately does not make this API global so we need
	// to pull it in if we don't have it.
	// eslint-disable-next-line @typescript-eslint/no-var-requires
	globalThis.File = require('undici').File;
	} catch (e) {
	throw new Error('The File API is required for this library. https://developer.mozilla.org/en-US/docs/Web/API/File');
	}
	}

Conversation

kanadgupta commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧰 Changes

🧬 QA & Testing

Uh oh!

rafegoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rafegoldberg Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domharrington left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erunion left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kanadgupta commented Sep 21, 2023 •

edited

Loading

rafegoldberg Sep 26, 2023 •

edited

Loading