Skip to content
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

Ensure that ignoreUrlParametersMatching works with directoryIndex #762

Merged
merged 4 commits into from
Sep 6, 2017

Conversation

jeffposnick
Copy link
Contributor

R: @addyosmani @gauntface

Fixes #758

This fixes a small bug due to using the wrong variable inside of our WorkboxSW precache route.

@jeffposnick jeffposnick added the Bug An issue with our existing, production codebase. label Aug 22, 2017
@jeffposnick
Copy link
Contributor Author

jeffposnick commented Aug 22, 2017

Something about the new test is causing Firefox to crash... tracking that down now.

Fixed.

Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you simplify the test at all? Reading it with the template literals everywhere I got really confused with that is the input and output . The actual fix makes total sense.

Copy link
Member

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the actual fix to workbox-sw.

I share Matt's concerns about the complexity of the testing setup and if there's anything that can be done to improve this that would be appreciated.

@jeffposnick
Copy link
Contributor Author

I've added some additional comments and refactored some of the variables, and hopefully it's mergeable now. I had initially tried to come up with a more straightforward way of testing the actual functionality, and what I ended up with was about as direct as I could figure out.

(Stubbing out some of the Route methods didn't seem possible, as they're not exposed anywhere in the public interface for WorkboxSW.)

promiseChain.then(() => {
// Make sure that the cache.match() stub was called, indicating that
// there was a match against the precached URL.
if (stub.called) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is caches.match called with? Should it be with or without the directoryIndex value or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in a check.

// Request the URL without the directory index, but with the URL param.
request: new Request(urlToRequest),
});
fetchEvent.respondWith = (promiseChain) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you stub this out? Would make it a little easier to read / follow if you could

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I can stub out respondWith() and still get things to work, no. (I copied this pattern from other tests in this suite, e.g. the previous test in this file.)

Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could add a check that the stub was called with the correct args that would be the only change I'd like to see, the other suggestion is a nit.

@jeffposnick jeffposnick merged commit 30f09c6 into master Sep 6, 2017
@jeffposnick jeffposnick deleted the ignore-url-params-and-directory-index branch September 6, 2017 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug An issue with our existing, production codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using ignoreUrlParametersMatching has a bug when testing with directoryIndex
3 participants