client/modules/Preview: migrate to TypeScript, no-verify#3879
client/modules/Preview: migrate to TypeScript, no-verify#3879NalinDalal wants to merge 13 commits intoprocessing:developfrom
client/modules/Preview: migrate to TypeScript, no-verify#3879Conversation
Release EnvironmentsThis Environment is provided by Release, learn more! 🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-a804a46e79 |
yugalkaushik
left a comment
yugalkaushik
left a comment
There was a problem hiding this comment.
Hi @NalinDalal Thank you for the PR. The type annotations are clean and the file-by-file conversion makes sense. There are a couple issues with the code that needs to be fixed, I pointed out few myself but still there can be multiple problem. So after the requested changes please understand the structure and check nothing is broken with proper testing.
| function PreviewApp() { | ||
| const [files, setFiles] = useState<File[]>([]); | ||
| const [isPlaying, setIsPlaying] = useState(false); | ||
| const [basePath, setBasePath] = useState('/'); |
There was a problem hiding this comment.
The old code had useState('') (empty string). Changing the default to '/' is a silent behaviour change, unless you thought of intentional reason for this change it should stay '' to match the original.
| useEffect(() => { | ||
| function handleMessage(message: any) { | ||
| switch (message.type) { | ||
| case MessageTypes.REGISTER: |
There was a problem hiding this comment.
The MessageTypes.EXECUTE case from the original previewIndex.jsx is completely missing please check it out.
| } | ||
| } | ||
|
|
||
| function addCacheBustingToAssets(files) { |
There was a problem hiding this comment.
This block is completely missing from previewIndex.tsx, please checkout the entire file I can still see many issues.
| @@ -8,6 +8,41 @@ declare module '*.svg' { | |||
| export default ReactComponent; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
There's a conflict here. You're adding types/blob-util in package.json AND manually declaring blob-util in custom.d.ts. These two type sources will conflict.
either remove the manual declare module 'blob-util' block from custom.d.ts and let @types/blob-util handle it, or drop @types/blob-util from package.json and fix the declaration here to include a default export.
client/tsconfig.json
Outdated
| "jsx": "react", | ||
| }, | ||
| "include": ["./**/*"], | ||
| "types": ["node"] }, |
There was a problem hiding this comment.
The formatting here is broken please fix it
There was a problem hiding this comment.
This file should not be in this PR
| } | ||
|
|
||
| export function filesReducer(state, action) { | ||
| export function filesReducer(state: FileState, action: FilesAction): FileState { |
There was a problem hiding this comment.
The filesReducer function is still exported but previewIndex.tsx no longer uses useReducer, it uses plain useState + createBlobUrls directly.
2 participants
Fixes #3828
Changes:
I have verified that this pull request:
npm run lint)npm run test)npm run typecheck)developbranch.Fixes #123