-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Puppeteer Adapter #64
Conversation
offirgolan
commented
Jul 15, 2018
•
edited
Loading
edited
- Puppeteer Adapter
- Refactor tests so they are re-usable by all adapters/persisters
|
||
parsedUrl.set('query', sortedQuery); | ||
if (isObjectLike(parsedUrl.query)) { | ||
parsedUrl.set('query', parse(stringify(parsedUrl.query))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realized the stable stringify will do all the hard work for us 😄
statusText, | ||
statusCode: status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a similar api to our Response class.
// import persisterTests from '@pollyjs-tests/integration/persister-tests'; | ||
// import { setupMocha as setupPolly, FetchAdapter } from '@pollyjs/core'; | ||
|
||
// // TODO: re-enable once the fetch adapter can accept a context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: re-enable once the fetch adapter can accept a context
@@ -18,7 +19,7 @@ export default function createBrowserTestConfig(options = {}) { | |||
`, | |||
outro: '});' | |||
}, | |||
plugins: [multiEntry()] | |||
plugins: [alias({ '@pollyjs-tests': testsPath }), multiEntry()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an alias to our common tests
directory so we dont have to use relative paths everywhere.
if (requestResourceTypes.includes(request.resourceType())) { | ||
this.handleRequest({ | ||
url: request.url(), | ||
method: request.method() || 'GET', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fallback required?
} | ||
|
||
async _passthroughRequest(pollyRequest) { | ||
const response = await fetch(pollyRequest.url, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will lead to unexpected behavior given the different nature of node-fetch vs browser fetch. I.e., credentials
headers: { | ||
exclude: [ | ||
// We don't want new recordings when we update chrome | ||
'user-agent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we default?
Hey there @offirgolan - this looks amazing! Wondering if this will close #19? |
@shanemcgraw thanks! Im not entirely sure if that will fully resolve #19. I'll leave it open in the mean time until I'm more certain. |
I landed the parts that enable Jest to work. I kept the issue open until we document the jest setup process and add test coverage before we officially advertise it's supported. Some of that context is in the issue regarding setting up. PRs welcome if anyone wants to sheppard these changes. Otherwise, likely next on my list. |