chore(protocol): split channel and dispatcher types#41321
Conversation
| eventTypes.push({eventName, eventType: paramsName}); | ||
| const dispatcherParamsName = `${channelName}${titleCase(eventName)}DispatcherEvent`; | ||
| ts_types.set(paramsName, channelParameters.ts); | ||
| ts_types.set(dispatcherParamsName, dispatcherParameters.ts); |
There was a problem hiding this comment.
I think it would be fair to generate src/client/channels.d.ts and src/server/channels.d.ts as two separate files. This way we can also avoid duplicating all the code here in the generator, and instead call generate() function twice with different arguments.
| } | ||
|
|
||
| channels_ts.push(` ${methodName}(params${method.parameters ? '' : '?'}: ${paramsName}, progress?: Progress): Promise<${resultName}>;`); | ||
| channelMethods.push(` ${methodName}(params${method.parameters ? '' : '?'}: ${channelParamsName}, signal?: AbortSignal): Promise<${channelResultName}>;`); |
There was a problem hiding this comment.
I think adding signal should be done in another PR 😄
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Skn0tt
force-pushed
the
proto-dispatcher-split
branch
from
cb538f2 to
d3a0b6e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| import type { Language } from '@isomorphic/locatorGenerators'; | ||
| import type { AttributeSelectorPart, NestedSelectorBody, ParsedSelector, ParsedSelectorPart } from '@isomorphic/selectorParser'; | ||
| import type * as channels from '@protocol/channels'; | ||
| import type * as channels from '../../playwright-core/src/client/channel'; |
There was a problem hiding this comment.
Why is this one using client channels instead of server channels? Even better - let's not use channels here, and declare an explicit type?
| import { parseEvaluationResultValue, serializeAsCallArgument } from '@isomorphic/utilityScriptSerializers'; | ||
|
|
||
| import type * as channels from '@protocol/channels'; | ||
| import type * as channels from '../../playwright-core/src/client/channel'; |
| import type { ActionTraceEvent } from '@trace/trace'; | ||
| import type { ActionEntry, ContextEntry, PageEntry } from './entries'; | ||
| import type { StackFrame } from '@protocol/channels'; | ||
| import type { StackFrame } from '../../playwright-core/src/client/channel'; |
There was a problem hiding this comment.
This one can also use a custom type, that's fine.
| */ | ||
|
|
||
| import type { ClientSideCallMetadata, StackFrame } from '@protocol/channels'; | ||
| import type { ClientSideCallMetadata, StackFrame } from '../../playwright-core/src/client/channel'; |
|
|
||
| import { isRegExp, isString } from './rtti'; | ||
| import type { ExpectedTextValue } from '@protocol/channels'; | ||
| import type { ExpectedTextValue } from '../playwright-core/src/client/channel'; |
There was a problem hiding this comment.
This file does not make sense anymore, because one caller wants to produce client-side options, and another wants to produce server-side options.
We can probably make all these random imports work by generating a single @protocol/channels with options/params, and two different client/server files for the actual FooBar interfaces. What do you think? Sorry for going back and worth.
| */ | ||
|
|
||
| import type { SerializedError } from './channels'; | ||
| import type { SerializedError } from './channel'; |
There was a problem hiding this comment.
Love this file move! That said, we can inline it into packages/playwright-core/src/server/instrumentation.ts right away.
| import type { CallMetadata, SdkObject } from './instrumentation'; | ||
|
|
||
| export type { Progress } from '@protocol/progress'; | ||
| export interface Progress { |
| import type { FullConfig, Location } from '../../types/testReporter'; | ||
| import type { config as commonConfig, FullConfigInternal, test as testNs } from '../common'; | ||
| import type { StackFrame } from '@protocol/channels'; | ||
| import type { StackFrame } from '../../../playwright-core/src/client/channel'; |
There was a problem hiding this comment.
I find it surprising that we use the same StackFrame in so many places.
| } | ||
| channels_ts.push(` undefined;`); | ||
| channels_ts.push(``); | ||
| function generateChannels(target) { |
There was a problem hiding this comment.
I hope this is just a big indentation change. Otherwise, it's hard to review 😄
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"7341 passed, 1122 skipped Merge workflow run. |
Test results for "tests 1"7 flaky39590 passed, 743 skipped Merge workflow run. |
2 participants
Summary