Skip to content

Conversation

@AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Mar 13, 2024

attempting to unblock #10590

Right now the feedback PR fails ts3.8 e2e tests: https://github.com/getsentry/sentry-javascript/actions/runs/8268834386/job/22630054663?pr=10590

This is because of TS issues, preact types do not work well with older TS types:

> @sentry-internal/ts3.8-test@ type-check /home/runner/work/sentry-javascript/sentry-javascript/dev-packages/e2e-tests/test-applications/generic-ts3.8
> tsc --project tsconfig.json

##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts'. Path does not exist
Error: node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts(1462,3): error TS2304: Cannot find name 'SubmitEvent'.
##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts'. Path does not exist
Error: node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts(1479,25): error TS2304: Cannot find name 'PictureInPictureEvent'.
##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts'. Path does not exist
Error: node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts(2549,65): error TS2304: Cannot find name 'MathMLElement'.
##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts'. Path does not exist
Error: node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts(2565,49): error TS2304: Cannot find name 'MathMLElement'.
##[debug]Dropping file value '/home/runner/work/sentry-javascript/sentry-javascript/node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts'. Path does not exist
Error: node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts(2571,52): error TS2304: Cannot find name 'MathMLElement'.

Essentially these types don't work on older versions and because the feedback package is depended on by the browser package, we will break a certain set of our users. We have to fix this before we can ship because of the browser package dependency.

To get around this, we do a workaround with packages/feedback/scripts/shim-preact-export.js. Essentially we shim the preact type in our exports, which should change what the declaration files themselves point to. So something like the following:

import type { ComponentChildren, VNode } from 'preact';

turns into

// no preact to be seen!
type ComponentChildren: any;
type VNode: any;

@AbhiPrasad AbhiPrasad changed the base branch from develop to cl/screenshot-integration March 13, 2024 22:32
@AbhiPrasad AbhiPrasad changed the title Abhi screenshot feedback preact Try aliasing preact typescript declaration to fix ts errors Mar 13, 2024
@AbhiPrasad AbhiPrasad force-pushed the abhi-screenshot-feedback-preact branch 2 times, most recently from 851fd44 to 38e4b97 Compare March 13, 2024 22:42
@AbhiPrasad AbhiPrasad closed this Mar 13, 2024
@AbhiPrasad AbhiPrasad reopened this Mar 13, 2024
@AbhiPrasad AbhiPrasad marked this pull request as ready for review March 13, 2024 22:42
@AbhiPrasad AbhiPrasad changed the base branch from cl/screenshot-integration to develop March 13, 2024 22:42
@AbhiPrasad
Copy link
Member Author

AbhiPrasad commented Mar 13, 2024

changing branches for CI to run, I'll cherry-pick this commit directly on cl/screenshot-integration if this works!

nvm CI sucks I'll stay on this branch.

@AbhiPrasad AbhiPrasad force-pushed the abhi-screenshot-feedback-preact branch from 38e4b97 to 470cc40 Compare March 13, 2024 22:43
@AbhiPrasad AbhiPrasad changed the title Try aliasing preact typescript declaration to fix ts errors [DO NOT MERGE] Try aliasing preact typescript declaration to fix ts errors Mar 13, 2024
walk(filePath);
} else {
if (filePath.endsWith('.d.ts')) {
const content = fs.readFileSync(filePath, 'utf8');

Check failure

Code scanning / CodeQL

Potential file system race condition

The file may have changed since it [was checked](1).
const content = fs.readFileSync(filePath, 'utf8');
if (preactImportRegex.test(content)) {
const newContent = content.replace(preactImportRegex, '// replaced import from preact');
fs.writeFileSync(filePath, snippet + newContent, 'utf8');

Check failure

Code scanning / CodeQL

Potential file system race condition

The file may have changed since it [was checked](1).
@AbhiPrasad AbhiPrasad changed the title [DO NOT MERGE] Try aliasing preact typescript declaration to fix ts errors Try aliasing preact typescript declaration to fix ts errors Mar 14, 2024
@AbhiPrasad AbhiPrasad changed the base branch from develop to cl/screenshot-integration March 14, 2024 03:46
const content = fs.readFileSync(filePath, 'utf8');
const capture = preactImportRegex.exec(content);
if (capture) {
const groups = capture[1].split(',').map(s => s.trim());
Copy link
Member Author

@AbhiPrasad AbhiPrasad Mar 14, 2024

Choose a reason for hiding this comment

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

shrewd readers would notice that this breaks completely with multiline imports

import {
  x,
  y,
  z,
} from 'preact';

I know, but we can cross that bridge when we come to it - prefer to get this out to unblock asap.

@AbhiPrasad AbhiPrasad merged commit 4591603 into cl/screenshot-integration Mar 14, 2024
@AbhiPrasad AbhiPrasad deleted the abhi-screenshot-feedback-preact branch March 14, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants