Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Preact typescript pragma fix #15564

Merged
merged 11 commits into from
Jul 27, 2022
Merged

Preact typescript pragma fix #15564

merged 11 commits into from
Jul 27, 2022

Conversation

KrofDrakula
Copy link
Member

Issue:

What I did

  • Updated Babel configuration to use automatic runtime and set importSource to preact by default. This does not require importing  h from preact, and importing React works for compatibility.
  • Updated tsconfig to take account of default JSX factories in preact-kitchen-sink.
  • Pragmas should not be required in story files.
  • Fixed stories to reflect new rules.

How to test

  • Is this testable with Jest or Chromatic screenshots? ✅ Yessir.
  • Does this need a new example in the kitchen sink apps? ✅ Already contains one.
  • Does this need an update to the documentation? ❌ There are no current Preact snippets available.

@nx-cloud
Copy link

nx-cloud bot commented Jul 13, 2021 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 01522da. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@KrofDrakula
Copy link
Member Author

@shilman Storyshots seem to fail after updating the Webpack plugin configuration to make Storybook run, but I'm unsure on how to solve the storyshots addon problems. Any pointers?

@KrofDrakula KrofDrakula linked an issue Jul 15, 2021 that may be closed by this pull request
storybook cli doesn't add @babel/typescript when scaffolding preact project #15531
Closed
@shilman shilman added the preact label Jul 15, 2021
@darleendenno darleendenno self-assigned this Jul 19, 2021
@shilman shilman added this to the 6.4 PRs milestone Jul 22, 2021
@darleendenno darleendenno removed their assignment Sep 23, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jan 9, 2022
@shilman shilman removed this from the 6.4 PRs milestone May 20, 2022
@stale stale bot removed the inactive label May 20, 2022
@ndelangen ndelangen self-assigned this Jul 6, 2022
@ndelangen ndelangen self-requested a review July 6, 2022 15:55
@ndelangen
Copy link
Member

Hey @KrofDrakula This looks really good! Thank you for this PR. I'm real sorry it's been left without any attention for so long..

I merged in next (4000+ commits behind, wow!), and I notice that the storyshots test is failing.. Could you look at this? I assume there's some babel config trickery that need the be updated to make it work.

Happy to merge once that's fixed!

@KrofDrakula
Copy link
Member Author

@ndelangen I'll have a look; it's been a while since I dug into this. :)

@ndelangen
Copy link
Member

Yes I understand, mind that a dirty hack to get things working is totally acceptable here, because we're replacing storyshots with the storybook test-runner.

Storyshots will not survive much longer, but we also don't want it to break by accident.

@KrofDrakula
Copy link
Member Author

@ndelangen Finally caught some time to fix this. The underlying issue was that Jest wasn't transpiling JSX correctly (using React.createElement instead of h as is customary for Preact) and TypeScript wasn't setup correctly for Jest transpilation. I've renamed the files to JS and all the Jest tests pass now since introducing a Babel overridefor the Preact kitchen sink example. This means that TSX won't work currently compile correctly without more work.

Also, I've had to reintroduce the h import, but that's idiomatic within the Preact community anyways, so I felt that was OK.

Let me know if there's anything that may need a closer look at.

@KrofDrakula
Copy link
Member Author

KrofDrakula commented Jul 22, 2022 •
edited
Loading

@ndelangen Actually I jumped the gun on that one, the issue is with TypeScript, so will charm up a fix soon.

@KrofDrakula
Copy link
Member Author

@ndelangen OK, tests pass for storyshots and examples/preact-kitchen-sink now. There are some unrelated failures in snapshots for Angular since I merged the latest next branch, but I assume that's currently expected? I may need some help on that if we decide we should address those too in this PR.

@KrofDrakula KrofDrakula mentioned this pull request Jul 22, 2022
Closed
@KrofDrakula KrofDrakula requested a review from ndelangen July 22, 2022 20:15
@KrofDrakula KrofDrakula mentioned this pull request Jul 24, 2022
Closed
@ndelangen
Merge branch 'next' into preact-typescript-pragma-fix
e3115fd 
# Conflicts:
#	app/preact/src/server/framework-preset-preact.ts
#	code/examples/preact-kitchen-sink/.storybook/main.ts
#	code/examples/preact-kitchen-sink/src/stories/test-cases/__snapshots__/test-component.stories.storyshot
#	code/examples/preact-kitchen-sink/src/stories/test-cases/no-pragma.tsx
#	code/examples/preact-kitchen-sink/src/stories/test-cases/test-component.stories.tsx
#	code/examples/preact-kitchen-sink/tsconfig.json
ndelangen
ndelangen reviewed Jul 27, 2022
View reviewed changes
code/examples/preact-kitchen-sink/tsconfig.json
Comment on lines +4 to +5
"jsxFactory": "h",
"jsxFragmentFactory": "Fragment",
Copy link
Member

Choose a reason for hiding this comment

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

This seems to give me error/warnings when I open the file:
Screen Shot 2022-07-27 at 11 31 24

@ndelangen
Copy link
Member

@KrofDrakula I updated the branch with next which had some pretty drastic changes.
Could you have a look at the PR changed file, to see if I made the right calls in resolving the merge conflicts? 🙏 Thanks!

To me this PR looks good, if the CI checks are 🟢 we can merge!

@KrofDrakula
Copy link
Member Author

@ndelangen Thanks, will have a look. Yeah, I wasn't too sure about the tsconfig change, the editor error might be because of mismatching TS runtime versions (I'm using the latest one vs. the version specified in package.json for the Storybook repo, which is pretty old).

I'll have to look into that one; I'm always confused about that compile option, as it doesn't quite align with the Babel JSX plugin config.

@ndelangen ndelangen merged commit 3e0d68b into next Jul 27, 2022
@ndelangen ndelangen deleted the preact-typescript-pragma-fix branch July 27, 2022 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndelangen ndelangen ndelangen approved these changes

Assignees

@ndelangen ndelangen

Labels
bug preact
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

storybook cli doesn't add @babel/typescript when scaffolding preact project
4 participants
@KrofDrakula @ndelangen @shilman @darleendenno