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

Fix Issue #80 inserting every fontUsage (including unused font) for multiple html target #81

Closed
wants to merge 3 commits into from

Conversation

palindrom615
Copy link
Contributor

No description provided.

@palindrom615 palindrom615 changed the title Fix Issue #80: inserting every fontUsage (including unused font) for multiple html target Fix Issue #80 inserting every fontUsage (including unused font) for multiple html target Feb 29, 2020
@coveralls
Copy link

coveralls commented Feb 29, 2020

Pull Request Test Coverage Report for Build 290

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 89.176%

Files with Coverage Reduction New Missed Lines %
lib/HeadlessBrowser.js 2 91.43%
Totals Coverage Status
Change from base Build 288: -0.2%
Covered Lines: 979
Relevant Lines: 1050

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 290

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 89.176%

Files with Coverage Reduction New Missed Lines %
lib/HeadlessBrowser.js 2 91.43%
Totals Coverage Status
Change from base Build 288: -0.2%
Covered Lines: 979
Relevant Lines: 1050

💛 - Coveralls

Copy link
Collaborator

@papandreou papandreou left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, and sorry for the lack of feedback until now.

It's a bit hard to tell if it's a legit issue -- could you provide a failing test for the current state?

// https://gitter.im/assetgraph/assetgraph?at=5dbb6438a3f0b17849c488cf
it('should not short circuit because the first page does not need any subset fonts', async function() {
it('should not short circuit with explicit subsetPerPage:true option because the first page does not need any subset fonts', async function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably deserves to be its own test case alongside the existing one.

However, if I copy your test over to master, it also passes? Can you come up with an actual regression test?

htmlAssetTextsWithProps,
htmlAssetTextWithProps => htmlAssetTextWithProps.fontUsages
);
const globalFontUsageMap = fontUsagesConcat.reduce(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind turning this reduce construct into a regular for...of loop? I find those much easier to reason about.

fontUsageMap[fontUsage.fontUrl].text + fontUsage.text
)
.sort()
.join('');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, mental note that we should find a better way achieve the "unique across pages" thing, sorting the output of _.uniq breaks when there's multiple non-UTF-16 characters:

require('lodash').uniq('🤗').sort().join('')
'🤗'
require('lodash').uniq('🤗🤞').sort().join('')
'🤗�'
require('lodash').uniq('🤗🤞👊').sort().join('')
'�🡊��'

Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't have the same problem. Not very readable though:

[...new Set([...'🤗🤞👊'])].sort().join('')

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! I think we should go with that, then. Not readable is better than broken :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Implemented your fix here: #87

@papandreou
Copy link
Collaborator

Replaced by #111

@papandreou papandreou closed this Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants