Skip to content

pr05 Typescript #2: automatically resolve imports of ts files & migrate instance of client/utils file #3540

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

Conversation

clairep94
Copy link
Collaborator

@clairep94 clairep94 commented Jul 15, 2025

pr05 Typescript Migration: Auto-resolve import of ts files & migrate instance of client/utils file

IMPORTANT: Should be reviewed after #3533 for clarity

Context:

  • Resolve issue with needing to update import extensions (eg. from js to ts), so that ts imports automatically resolve for both the client folder & tests
    • Not sure if this resolves imports for the server folder yet -- will resolve this on the PR to migrate an instance of a server file later if so
  • Migrate instance of a client/utils file

Changes:

  • Auto-resolve imports for local instance of project client folder
  • Auto-resolve imports for jest tests
  • Migrate client/utils/generateRandomName & associated mock
  • Delete client/utils/isSecurePage -- checked and this utility function is unused in the project

Notes:

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@clairep94 clairep94 marked this pull request as ready for review July 16, 2025 19:02
@clairep94
Copy link
Collaborator Author

clairep94 commented Jul 16, 2025

@khanniie @raclim

Should be reviewed after #3533 is merged for clarity

Copy link
Collaborator

@khanniie khanniie left a comment

Choose a reason for hiding this comment

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

LGTM to me so far but would love to re-approve once the first PR is in just to see the diff without the forked changes!

Copy link
Collaborator

@khanniie khanniie left a comment

Choose a reason for hiding this comment

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

thanks so much!!! looking great!

@@ -114,6 +118,7 @@
"@svgr/webpack": "^6.2.1",
"@testing-library/jest-dom": "^5.15.0",
"@testing-library/react": "^12.1.2",
"@types/friendly-words": "^1.2.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this, if we're not using the types yet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this may be a misunderstanding but i thought we'd only need it if we did something like import {aType} from '@types/sdfdsf' to use aType later on?

Copy link
Collaborator Author

@clairep94 clairep94 Jul 27, 2025

Choose a reason for hiding this comment

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

Yess that's a good point!! I did some searching to double-check on this bc we also have "skipLibCheck": true(https://github.com/processing/p5.js-web-editor/blob/develop/tsconfig.base.json#L7), which should allow us to not have to have declaration files within packages in node_modules, but I think it works the following way:

So I think this is expected, and as we go we will likely have to continue installing @types packages for any third-party packages we are using in a file that is being migrated (eg. react-router-dom, jest-react etc)

https://stackoverflow.com/questions/59906323/typescript-skiplibcheck-still-checking-node-modules-libs

Copy link
Collaborator

@khanniie khanniie left a comment

Choose a reason for hiding this comment

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

thank you claire!!

Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks so much @clairep94 and @khanniie!

@raclim raclim merged commit 00555ca into processing:develop Jul 28, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr05 Grant Projects pr05 Grant Projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants