Golfing #15
Merged
Golfing #15
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
- 886 gz / 734 br
- 883 gz / 726 br
- 883 gz / 723 br
- 880 gz / 717 br
- 875 gz / 720 br
- 873 gz / 719 br
- 870 gz / 717 br
- 868 gz / 711 br
- is only low vs high - 832 gz / 673 br
- 826 gz / 669 br
- 786 gz / 628 br
- 780 gz / 626 br
- 764 gz / 620 br
- 748 gz / 612 br
- can only enter `linkPrefetchStrategy()` if `support()` passed – which includes same check - 735 gz / 601 br
addyosmani
reviewed
Dec 11, 2018
| @@ -42,14 +42,14 @@ function prefetcher(url) { | |||
| * @param {Object} options - Configuration options for quicklink | |||
| * @param {Array} options.urls - Array of URLs to prefetch (override) | |||
| * @param {Object} options.el - DOM element to prefetch in-viewport links of | |||
| * @param {string} options.priority - Attempt to fetch with higher priority (low or high) | |||
| * @param {Boolean} options.priority - Attempt higher priority fetch (low or high) | |||
There was a problem hiding this comment.
I initially planned for priority to have a tri (or more) state set of values, but it's very likely to stay at either high or low. With that in mind, strongly in favor of switching to a Boolean here 👍
There was a problem hiding this comment.
Ah, gotcha. I didn't even know there could be other priorities 😆
I think this snuck in after adopting microbundle 😋. I'll have to fix up or ignore after landing this change. |
|
Haha, it's happened to me before too. I can fix it up for you after if you'd like. |
addyosmani
approved these changes
Dec 11, 2018
There was a problem hiding this comment.
LGTM. Thanks for working on all of these ⛳️ improvements, @lukeed!
addyosmani
added a commit
that referenced
this pull request
Dec 11, 2018
This change fixes some of the mixed tabs and spaces from the last PR. Unfortunately ESLint doesn't support fixing these automatically (so npm run lint-fix doesn't help here). We should probably fix the editorconfig soonish :)
|
Awesome, thank you! 🙇 |
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.
Sorry for the long-ish PR, but I wanted to break apart each step to document its savings as well make it easier to discuss/revert individual changes. If this is to be accepted, I'd recommend squashing.
The core functionality is preserved at 735 B (gz) / 601 B (br) – down from 902 B (gz) / 747 B (br). It's possible to get it down to
717 gz / 594 brby inlining theoptionsdefaults, but that may not read as well 😄There is 1 breaking change (which, of course, I can revert): The
priorityoption is now a Boolean. Not only did this save ~30 bytes, but IMO it makes more sense since the only options were "high" vs "low". In this format,options.priority=trueis "high" priority.The other big wins came from:
AsyncFunctioninto a regular function that returned the Promiselink.setAttributes in favor of usinglink[key]directlydocument.headanddocument.querySelector