chore: make parent scope explicit #16819
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgozman
approved these changes
Aug 25, 2022
packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts
Show resolved
Hide resolved
packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts
Outdated
Show resolved
Hide resolved
packages/playwright-core/src/server/dispatchers/networkDispatchers.ts
Outdated
Show resolved
Hide resolved
packages/playwright-core/src/server/dispatchers/pageDispatcher.ts
Outdated
Show resolved
Hide resolved
pavelfeldman
force-pushed
the
explicit_scope
branch
from
7a72e6d
to
c0cbb6b
Compare
dgozman
reviewed
Aug 25, 2022
| @@ -62,7 +62,7 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel> im | |||
| return; | |||
| const params = { url: event.url, name: event.name, error: event.error ? event.error.message : undefined }; | |||
| if (event.newDocument) | |||
| (params as any).newDocument = { request: RequestDispatcher.fromNullable(this._scope, event.newDocument.request || null) }; | |||
| (params as any).newDocument = { request: RequestDispatcher.fromNullable(scope.parentScope()!, event.newDocument.request || null) }; | |||
There was a problem hiding this comment.
Shouldn't this be this._scope now that we put request as a child of frame?
There was a problem hiding this comment.
No, we are building the right scope based on the context scope inside the called method.
| super(scope, response, 'Response', { | ||
| // TODO: responses in popups can point to non-reported requests. | ||
| request: RequestDispatcher.from(scope, response.request()), | ||
| request: RequestDispatcher.from(contextScope, response.request()), |
There was a problem hiding this comment.
Shouldn't this use scope instead of contextScope?
There was a problem hiding this comment.
No, check out the called method.
| url: request.url(), | ||
| resourceType: request.resourceType(), | ||
| method: request.method(), | ||
| postData: postData === null ? undefined : postData, | ||
| headers: request.headers(), | ||
| isNavigationRequest: request.isNavigationRequest(), | ||
| redirectedFrom: RequestDispatcher.fromNullable(scope, request.redirectedFrom()), | ||
| redirectedFrom: RequestDispatcher.fromNullable(contextScope, request.redirectedFrom()), |
There was a problem hiding this comment.
scope instead of contextScope here as well?
| { _guid: 'frame', objects: [ | ||
| { _guid: 'request', objects: [] }, | ||
| { _guid: 'response', objects: [] }, | ||
| ] }, |
There was a problem hiding this comment.
I think adding more objects to this test like JSHandle, Route and Worker would be beneficial.
| super(scope, worker, 'Worker', { | ||
| url: worker.url() | ||
| }); | ||
| }, true); |
There was a problem hiding this comment.
It seems like we can declare every object a scope now. Do we have an example where it's not a scope and it matters?
There was a problem hiding this comment.
I might be able to do that in a follow-up.
7 tasks
rwoll
added a commit
to microsoft/playwright-python
that referenced
this pull request
Sep 20, 2022
Ports: - [x] microsoft/playwright@a07a4a2 (chore: make parent scope explicit (microsoft/playwright#16819)) - [x] microsoft/playwright@306ab34 (feat(assertions): support toBeEnabled({ enabled }) (microsoft/playwright#17058)) - [x] microsoft/playwright@f0c5810 (feat(assertions): support toBeEditable({ editable }) (microsoft/playwright#17065)) - [x] microsoft/playwright@bca13bc (feat(assertions): support toBeVisible({ visible }) (microsoft/playwright#17207)) - [x] microsoft/playwright@17b203a (feat: added follow and redirect arguments to fetch (microsoft/playwright#17033)) - [x] microsoft/playwright@fea8772 (fix: emit load/domcontentloaded events as reported by the browser) - [x] microsoft/playwright@01d83f1 (fix(har): record request overrides to har (microsoft/playwright#17027))
sbakerdev
added a commit
to sbakerdev/playwright-python
that referenced
this pull request
Nov 29, 2022
Ports: - [x] microsoft/playwright@a07a4a2 (chore: make parent scope explicit (microsoft/playwright#16819)) - [x] microsoft/playwright@306ab34 (feat(assertions): support toBeEnabled({ enabled }) (microsoft/playwright#17058)) - [x] microsoft/playwright@f0c5810 (feat(assertions): support toBeEditable({ editable }) (microsoft/playwright#17065)) - [x] microsoft/playwright@bca13bc (feat(assertions): support toBeVisible({ visible }) (microsoft/playwright#17207)) - [x] microsoft/playwright@17b203a (feat: added follow and redirect arguments to fetch (microsoft/playwright#17033)) - [x] microsoft/playwright@fea8772 (fix: emit load/domcontentloaded events as reported by the browser) - [x] microsoft/playwright@01d83f1 (fix(har): record request overrides to har (microsoft/playwright#17027))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.