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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| import type { AndroidServerLauncherImpl } from '../androidServerImpl'; | ||
| import type { Platform } from '@isomorphic/platform'; | ||
| import type * as channels from '@protocol/channels'; | ||
| import type * as channels from './channel'; |
There was a problem hiding this comment.
Just curious - why channel and not channel_s_?
There was a problem hiding this comment.
I also like channels more, updated.
| */ | ||
|
|
||
| import type { SerializedValue } from '@protocol/channels'; | ||
| import type { SerializedValue } from '../client/channel'; // same type across client and server, either is fine |
There was a problem hiding this comment.
This is also unfortunate, not sure what to do with it. Perhaps move to isomorphic? I don't think we can afford protocol depending on client or server.
There was a problem hiding this comment.
I duplicated the SerializedValue type into here, what do you think?
Test results for "MCP"1 failed 7353 passed, 1122 skipped Merge workflow run. |
Test results for "tests 1"7 flaky39602 passed, 743 skipped Merge workflow run. |
2 participants
Summary