Skip to content

Commit 06bc29a

Browse files
authored
Resolving highlight client from firstload - attempt 2 (#4958)
## Summary <!-- Ideally, there is an attached GitHub issue that will describe the "why". If relevant, use this section to call out any additional information you'd like to _highlight_ to the reviewer. --> This is another attempt at #4851, which had to be [reverted](#4950) because it broke type declarations. 😞 TypeScript isn't planning to officially support import path rewriting in declarations (see microsoft/TypeScript#31643 (comment)), so to fully support arbitrary path remappings (like the ones to `@highlight-run/client`), the most promising solution I could find involved a combination of [ttypescript](https://github.com./cevek/ttypescript), [ts-transform-paths](https://github.com./zerkalica/zerollup/tree/master/packages/ts-transform-paths), and [rollup-plugin-typescript2](https://github.com./ezolenko/rollup-plugin-typescript2). However in our case, we're only looking to remap a single reference, so a dumb script that looks for the reference and replaces them feels like a much simpler & lower risk solution. That's what I added in this PR in addition to the previously reverted changes. Let me know if y'all would prefer looking into the other approach instead, which might be worthwhile if we wanted to start using arbitrary path remappings in libraries (say, to be able use absolute imports everywhere). ## How did you test this change? <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> I ran `yarn build && yarn typegen` in both main and this branch, and ran `diff -rq` to compare both dist folders. There were no differences in any of the .d.ts files (indicating the import replacement script gave us the correct output), and the only differences were in `index.js`, `indexESM.js`, and their respective importmaps, due to the changes in the package.json file that ends up getting bundled. ![Screenshot 2023-04-12 at 4 43 37 PM](https://user-images.githubusercontent.com/6934200/231610022-ac690d7c-860b-419b-ba96-1287e296b491.png) ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> This time I did make sure to include type declarations in the build output comparison, so I have pretty high confidence they'll behave identically this time. Though one thing I couldn't confirm was whether the CI process that publishes this module runs the `yarn typegen` command or `yarn tsc` directly. The latter could produce the same problematic output as before since we depend on the script specified in `yarn typegen` to rewrite imports. Would appreciate it if someone could double check. 🙏
1 parent ad6504d commit 06bc29a

File tree

9 files changed

+62
-15
lines changed

9 files changed

+62
-15
lines changed

Diff for: sdk/firstload/package.json

+2-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
"dev": "rollup -c -w",
77
"enforce-size": "size-limit",
88
"test": "vitest --run",
9-
"typegen": "tsc"
9+
"typegen": "tsc && node scripts/replace-client-imports.mjs"
1010
},
1111
"main": "dist/index.js",
1212
"module": "dist/indexESM.js",
@@ -20,6 +20,7 @@
2020
"@size-limit/file": "^8.1.0",
2121
"@types/chrome": "^0.0.144",
2222
"esbuild": "^0.14.47",
23+
"readdirp": "^3.6.0",
2324
"rollup": "^2.79.1",
2425
"rollup-plugin-consts": "^1.1.0",
2526
"rollup-plugin-esbuild": "^4.9.1",

Diff for: sdk/firstload/rollup.config.js

+13-1
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,19 @@ const basePlugins = [
2222
consts({
2323
publicGraphURI: process.env.PUBLIC_GRAPH_URI,
2424
}),
25-
resolve({ browser: true }),
25+
resolve({
26+
browser: true,
27+
// @highlight-run/client is a private package not published to npm, so
28+
// listing it in package.json would break end users.
29+
// Instead, we add root node_modules as a resolution path so it gets
30+
// resolved as an internal module and included directly in the bundle.
31+
// An alternative to the previous solution using relative paths that
32+
// point outside package root described here:
33+
// https://www.highlight.io/blog/publishing-private-pnpm-monorepo
34+
modulePaths: ['../../node_modules'],
35+
// Need to override this to add .ts and .tsx as valid resolution exts.
36+
extensions: ['.mjs', '.js', '.json', '.node', '.ts', '.tsx'],
37+
}),
2638
webWorkerLoader({
2739
targetPlatform: 'browser',
2840
inline: true,

Diff for: sdk/firstload/scripts/replace-client-imports.mjs

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import readdirp from 'readdirp'
2+
import * as path from 'node:path'
3+
import * as fs from 'node:fs'
4+
5+
const workingDirectory = path.join(process.cwd(), './dist/firstload')
6+
const clientDirectory = path.join(process.cwd(), './dist/client')
7+
8+
const files = await readdirp.promise(workingDirectory, {
9+
fileFilter: '**/**.d.ts',
10+
})
11+
12+
const text = ` from '@highlight-run/client/`
13+
await Promise.all(
14+
files.map(async ({ fullPath }) => {
15+
const content = await fs.promises.readFile(fullPath, 'utf-8')
16+
17+
if (!content.includes(text)) return
18+
19+
return fs.promises.writeFile(
20+
fullPath,
21+
content.replaceAll(
22+
text,
23+
` from '${path.relative(
24+
path.dirname(fullPath),
25+
clientDirectory,
26+
)}/`,
27+
),
28+
)
29+
}),
30+
)

Diff for: sdk/firstload/src/index.tsx

+8-5
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,20 @@ import {
66
} from './integrations/amplitude'
77
import { MixpanelAPI, setupMixpanelIntegration } from './integrations/mixpanel'
88
import { initializeFetchListener } from './listeners/fetch'
9-
import { getPreviousSessionData } from '../../client/src/utils/sessionStorage/highlightSession'
10-
import { FirstLoadListeners } from '../../client/src/listeners/first-load-listeners'
11-
import { GenerateSecureID } from '../../client/src/utils/secure-id'
12-
import type { Highlight, HighlightClassOptions } from '../../client/src/index'
9+
import { getPreviousSessionData } from '@highlight-run/client/src/utils/sessionStorage/highlightSession'
10+
import { FirstLoadListeners } from '@highlight-run/client/src/listeners/first-load-listeners'
11+
import { GenerateSecureID } from '@highlight-run/client/src/utils/secure-id'
12+
import type {
13+
Highlight,
14+
HighlightClassOptions,
15+
} from '@highlight-run/client/src/index'
1316
import type {
1417
HighlightOptions,
1518
HighlightPublicInterface,
1619
Metadata,
1720
Metric,
1821
SessionDetails,
19-
} from '../../client/src/types/types'
22+
} from '@highlight-run/client/src/types/types'
2023
import HighlightSegmentMiddleware from './integrations/segment'
2124
import configureElectronHighlight from './environments/electron'
2225

Diff for: sdk/firstload/src/integrations/amplitude.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// @ts-nocheck
2-
import type { AmplitudeIntegrationOptions } from '../../../client/src/types/client'
3-
import type { Integration } from '../../../client/src/types/types'
2+
import type { AmplitudeIntegrationOptions } from '@highlight-run/client/src/types/client'
3+
import type { Integration } from '@highlight-run/client/src/types/types'
44

55
interface Window {
66
amplitude?: AmplitudeAPI

Diff for: sdk/firstload/src/integrations/mixpanel.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// @ts-nocheck
2-
import type { MixpanelIntegrationOptions } from '../../../client/src/types/client'
3-
import type { Integration } from '../../../client/src/types/types'
2+
import type { MixpanelIntegrationOptions } from '@highlight-run/client/src/types/client'
3+
import type { Integration } from '@highlight-run/client/src/types/types'
44

55
interface Window {
66
mixpanel?: MixpanelAPI

Diff for: sdk/firstload/src/integrations/segment.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { HighlightPublicInterface } from '../../../client/src/types/types'
1+
import type { HighlightPublicInterface } from '@highlight-run/client/src/types/types'
22

33
interface SegmentContext {
44
payload: any

Diff for: sdk/firstload/src/listeners/fetch/index.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import type { HighlightFetchWindow } from '../../../../client/src/listeners/network-listener/utils/fetch-listener'
2-
import type { HighlightPublicInterface } from '../../../../client/src/types/types'
1+
import type { HighlightFetchWindow } from '@highlight-run/client/src/listeners/network-listener/utils/fetch-listener'
2+
import type { HighlightPublicInterface } from '@highlight-run/client/src/types/types'
33

44
type HighlightWindow = Window & {
55
H: HighlightPublicInterface

Diff for: yarn.lock

+2-1
Original file line numberDiff line numberDiff line change
@@ -33832,6 +33832,7 @@ __metadata:
3383233832
"@size-limit/file": ^8.1.0
3383333833
"@types/chrome": ^0.0.144
3383433834
esbuild: ^0.14.47
33835+
readdirp: ^3.6.0
3383533836
rollup: ^2.79.1
3383633837
rollup-plugin-consts: ^1.1.0
3383733838
rollup-plugin-esbuild: ^4.9.1
@@ -46757,7 +46758,7 @@ __metadata:
4675746758
languageName: node
4675846759
linkType: hard
4675946760

46760-
"readdirp@npm:~3.6.0":
46761+
"readdirp@npm:^3.6.0, readdirp@npm:~3.6.0":
4676146762
version: 3.6.0
4676246763
resolution: "readdirp@npm:3.6.0"
4676346764
dependencies:

0 commit comments

Comments
 (0)