Skip to content

Commit

Permalink
Resolving highlight client from firstload - attempt 2 (highlight#4958)
Browse files Browse the repository at this point in the history
## 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
highlight#4851, which had to be
[reverted](highlight#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. 🙏
  • Loading branch information
lewisl9029 authored and shayneo committed Apr 19, 2023
1 parent 3cdcf40 commit 59247b5
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 15 deletions.
3 changes: 2 additions & 1 deletion sdk/firstload/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"dev": "rollup -c -w",
"enforce-size": "size-limit",
"test": "vitest --run",
"typegen": "tsc"
"typegen": "tsc && node scripts/replace-client-imports.mjs"
},
"main": "dist/index.js",
"module": "dist/indexESM.js",
Expand All @@ -20,6 +20,7 @@
"@size-limit/file": "^8.1.0",
"@types/chrome": "^0.0.144",
"esbuild": "^0.14.47",
"readdirp": "^3.6.0",
"rollup": "^2.79.1",
"rollup-plugin-consts": "^1.1.0",
"rollup-plugin-esbuild": "^4.9.1",
Expand Down
14 changes: 13 additions & 1 deletion sdk/firstload/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,19 @@ const basePlugins = [
consts({
publicGraphURI: process.env.PUBLIC_GRAPH_URI,
}),
resolve({ browser: true }),
resolve({
browser: true,
// @highlight-run/client is a private package not published to npm, so
// listing it in package.json would break end users.
// Instead, we add root node_modules as a resolution path so it gets
// resolved as an internal module and included directly in the bundle.
// An alternative to the previous solution using relative paths that
// point outside package root described here:
// https://www.highlight.io/blog/publishing-private-pnpm-monorepo
modulePaths: ['../../node_modules'],
// Need to override this to add .ts and .tsx as valid resolution exts.
extensions: ['.mjs', '.js', '.json', '.node', '.ts', '.tsx'],
}),
webWorkerLoader({
targetPlatform: 'browser',
inline: true,
Expand Down
30 changes: 30 additions & 0 deletions sdk/firstload/scripts/replace-client-imports.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import readdirp from 'readdirp'
import * as path from 'node:path'
import * as fs from 'node:fs'

const workingDirectory = path.join(process.cwd(), './dist/firstload')
const clientDirectory = path.join(process.cwd(), './dist/client')

const files = await readdirp.promise(workingDirectory, {
fileFilter: '**/**.d.ts',
})

const text = ` from '@highlight-run/client/`
await Promise.all(
files.map(async ({ fullPath }) => {
const content = await fs.promises.readFile(fullPath, 'utf-8')

if (!content.includes(text)) return

return fs.promises.writeFile(
fullPath,
content.replaceAll(
text,
` from '${path.relative(
path.dirname(fullPath),
clientDirectory,
)}/`,
),
)
}),
)
13 changes: 8 additions & 5 deletions sdk/firstload/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,20 @@ import {
} from './integrations/amplitude'
import { MixpanelAPI, setupMixpanelIntegration } from './integrations/mixpanel'
import { initializeFetchListener } from './listeners/fetch'
import { getPreviousSessionData } from '../../client/src/utils/sessionStorage/highlightSession'
import { FirstLoadListeners } from '../../client/src/listeners/first-load-listeners'
import { GenerateSecureID } from '../../client/src/utils/secure-id'
import type { Highlight, HighlightClassOptions } from '../../client/src/index'
import { getPreviousSessionData } from '@highlight-run/client/src/utils/sessionStorage/highlightSession'
import { FirstLoadListeners } from '@highlight-run/client/src/listeners/first-load-listeners'
import { GenerateSecureID } from '@highlight-run/client/src/utils/secure-id'
import type {
Highlight,
HighlightClassOptions,
} from '@highlight-run/client/src/index'
import type {
HighlightOptions,
HighlightPublicInterface,
Metadata,
Metric,
SessionDetails,
} from '../../client/src/types/types'
} from '@highlight-run/client/src/types/types'
import HighlightSegmentMiddleware from './integrations/segment'
import configureElectronHighlight from './environments/electron'

Expand Down
4 changes: 2 additions & 2 deletions sdk/firstload/src/integrations/amplitude.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @ts-nocheck
import type { AmplitudeIntegrationOptions } from '../../../client/src/types/client'
import type { Integration } from '../../../client/src/types/types'
import type { AmplitudeIntegrationOptions } from '@highlight-run/client/src/types/client'
import type { Integration } from '@highlight-run/client/src/types/types'

interface Window {
amplitude?: AmplitudeAPI
Expand Down
4 changes: 2 additions & 2 deletions sdk/firstload/src/integrations/mixpanel.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @ts-nocheck
import type { MixpanelIntegrationOptions } from '../../../client/src/types/client'
import type { Integration } from '../../../client/src/types/types'
import type { MixpanelIntegrationOptions } from '@highlight-run/client/src/types/client'
import type { Integration } from '@highlight-run/client/src/types/types'

interface Window {
mixpanel?: MixpanelAPI
Expand Down
2 changes: 1 addition & 1 deletion sdk/firstload/src/integrations/segment.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { HighlightPublicInterface } from '../../../client/src/types/types'
import type { HighlightPublicInterface } from '@highlight-run/client/src/types/types'

interface SegmentContext {
payload: any
Expand Down
4 changes: 2 additions & 2 deletions sdk/firstload/src/listeners/fetch/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { HighlightFetchWindow } from '../../../../client/src/listeners/network-listener/utils/fetch-listener'
import type { HighlightPublicInterface } from '../../../../client/src/types/types'
import type { HighlightFetchWindow } from '@highlight-run/client/src/listeners/network-listener/utils/fetch-listener'
import type { HighlightPublicInterface } from '@highlight-run/client/src/types/types'

type HighlightWindow = Window & {
H: HighlightPublicInterface
Expand Down
3 changes: 2 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -33832,6 +33832,7 @@ __metadata:
"@size-limit/file": ^8.1.0
"@types/chrome": ^0.0.144
esbuild: ^0.14.47
readdirp: ^3.6.0
rollup: ^2.79.1
rollup-plugin-consts: ^1.1.0
rollup-plugin-esbuild: ^4.9.1
Expand Down Expand Up @@ -46757,7 +46758,7 @@ __metadata:
languageName: node
linkType: hard

"readdirp@npm:~3.6.0":
"readdirp@npm:^3.6.0, readdirp@npm:~3.6.0":
version: 3.6.0
resolution: "readdirp@npm:3.6.0"
dependencies:
Expand Down

0 comments on commit 59247b5

Please sign in to comment.