chore: convert server/lib/plugins files from JS to TS #32337
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cypress
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
server-plugins-to-ts
|
| Run status |
|
| Run duration | 20m 21s |
| Commit |
|
| Committer | Jennifer Shehane |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
13
|
Pending
|
1102
|
Skipped
|
0
|
Passing
|
26661
|
| View all changes introduced in this branch ↗︎ | |
Warning
Partial Report: The results for the Application Quality reports may be incomplete.
UI Coverage
45.11%
|
|
|---|---|
|
Untested elements
|
186
|
|
Tested elements
|
157
|
Accessibility
97.71%
|
|
|---|---|
|
Failed rules
|
4 critical
8 serious
2 moderate
2 minor
|
|
Failed elements
|
110
|
| if (!(filePath in fileProcessors)) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Code used to be:
if (!fileProcessors[filePath]) {
return
}Since fileProcessors is typed as Record<string, Promise<string>>, the values are always Promise<string> objects, which are always truthy.
packages/server/lib/plugins/util.ts
Outdated
| import Promise from 'bluebird' | ||
| import path from 'path' | ||
| import type { CompilerErrorLocation, ProcessIpcWrapper, TransformError } from '@packages/types' | ||
| import type { SerializedError, ErrorLike } from '@packages/errors' |
There was a problem hiding this comment.
Pulling in error types from @packages/errors
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }, | ||
| } | ||
|
|
||
| export default API |
There was a problem hiding this comment.
any reason we have both module.exports and export default here @jennifer-shehane ? Since the code is internal we should just need export default API and any play this is required we just change the require('dev-server') to require('dev-server').default
There was a problem hiding this comment.
Yah I think they were being used in tests?
There was a problem hiding this comment.
This may be some leftover code from me trying to get the utils to work, I’ll revisit next week
There was a problem hiding this comment.
some tests fail with this change: https://app.circleci.com/pipelines/github/cypress-io/cypress/74232/workflows/357d572b-99d6-4b0a-944d-11a1e5e823e7/jobs/3106827/tests
There was a problem hiding this comment.
@jennifer-shehane it's likely stubbing something that is now exported on the default. i'll pull the branch down and check
|
|
||
| export default API | ||
|
|
||
| module.exports = API |
There was a problem hiding this comment.
| module.exports = API |
|
|
||
| export default API | ||
|
|
||
| module.exports = API |
There was a problem hiding this comment.
should be able to do the same here too. otherwise the compiled output could conflict
| execute: Promise.method((eventName: string, ...args: unknown[]) => { | ||
| if (!plugins.has(eventName)) return | ||
|
|
||
| return plugins.execute(eventName, ...args) |
There was a problem hiding this comment.
could be a good time to async/await this code but its out of scope
There was a problem hiding this comment.
I pushed up 09cb9b1 @jennifer-shehane that should fix the errors you are running into. inside the spec files, the imports now need to alias the default export since its loaded in a CJS context and is dynamically interpreting the TypeScript file
|
|
||
| export default API | ||
|
|
||
| module.exports = API |
There was a problem hiding this comment.
| module.exports = API |
| }, | ||
| } | ||
|
|
||
| export default API |
There was a problem hiding this comment.
@jennifer-shehane it's likely stubbing something that is now exported on the default. i'll pull the branch down and check
Additional details
Migrating server/lib/plugins files from JavaScript to TypeScript. No behavior is intended to change.
Few things to note:
server/lib/plugins/util.jsfile and....it is not ready to be touched basically. The problem seemed to be around the child process files needing the utils file to be JavaScript (to be spawnable).Interface Renaming for IPC comms:
IpcHandler→PluginIpcHandler(in@packages/types)send()returnsboolean,on()returnsthisfor chainingdev-server.ts,preprocessor.ts, and data-context plugin handlersIpcWrapper→ProcessIpcWrapper(inutil.ts)send()returnsvoid,on()returnsEventEmitter, plusremoveListener()util.tsfor wrapping child processesSteps to test
How has the user experience changed?
N/A
PR Tasks
cypress-documentation?type definitions?