Skip to content

Try aliasing preact typescript declaration to fix ts errors #11088

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

Merged
merged 4 commits into from
Mar 14, 2024

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.

2 participants