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

Fixed an issue with prefixer going into an infinite recursion #244

Merged
merged 2 commits into from
Dec 1, 2020

Conversation

Andarist
Copy link
Collaborator

It's something that has been reported here: emotion-js/emotion#2117

Gonna think about a potential fix for this later.

@coveralls
Copy link

coveralls commented Nov 18, 2020

Pull Request Test Coverage Report for Build 00a729afc4b3893f7f1f2f0a5e5201b82e224521-PR-244

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 762c50a10a4eb0db3f132cc731d96d8c8156c9b6: 0.0%
Covered Lines: 237
Relevant Lines: 237

💛 - Coveralls

src/Prefixer.js Outdated
@@ -63,18 +63,19 @@ export function prefix (value, length) {
case 8116: case 7059: case 5753: case 5535:
case 5445: case 5701: case 4933: case 4677:
case 5533: case 5789: case 5021: case 4765:
var temp = value
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not ideal, but introducing a temp is the best I could figure out for this 🤷‍♂️

src/Prefixer.js Outdated
// (s)tretch
case 115:
return prefix(replace(value, 'stretch', 'fill-available'), length) + value
temp = (value = replace(value, 'stretch', 'fill-available')) + temp
Copy link
Owner

Choose a reason for hiding this comment

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

return indexof(value, 'stretch') > -1 ? ... : value

Copy link
Owner

@thysultan thysultan Nov 18, 2020

Choose a reason for hiding this comment

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

Or you can just copy the fill-available case here

replace(value, /(.+:)(.+)-([^]+)/, '$1' + WEBKIT + '$2-$3' + '$1' + MOZ + '$3') + value

IGNORE

Copy link
Collaborator Author

@Andarist Andarist Nov 18, 2020

Choose a reason for hiding this comment

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

the indexof approach doesnt work because of this fit-content:
https://github.com/thysultan/stylis.js/blob/9799734dcf2b04c1216818f8b74cba92b6de4f76/test/Prefixer.js#L130

this is actually missing from the comment above case 102:, should be:

// (f)ill-available, (f)it-content

we could try to use match or whatever but I don't think that testing for specific cases is scalable here, Stylis assumes valid input and this is IMHO OK, the goal of this PR to just be a little bit more fault-tolerant

Copy link
Owner

Choose a reason for hiding this comment

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

This should work

// (min|max)?(width|height|inline-size|block-size)
case 8116: case 7059: case 5753: case 5535:
case 5445: case 5701: case 4933: case 4677:
case 5533: case 5789: case 5021: case 4765:
	// stretch, max-content, min-content, fill-available
	if (strlen(value) - 1 - length > 6)
		switch (charat(value, length + 1)) {
			// (f)ill-available, (f)it-content
			case 102: length = charat(value, length + 3)
			// (m)ax-content, (m)in-content
			case 109:
				return replace(value, /(.+:)(.+)-([^]+)/, '$1' + WEBKIT + '$2-$3' + '$1' + MOZ + (length == 108 ? '$3' : '$2-$3')) + value
			// (s)tretch
			case 115:
				return ~indexof(value, 'stretch') ? prefix(replace(value, 'stretch', 'fill-available'), length) + value : value
		}
	break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, went with that one

Copy link
Owner

Choose a reason for hiding this comment

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

Did you see the case 102: length = charat(value, length + 3) amendment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, no - I've focused on the indexof part in a hurry. This is supposed only to compress the code, right? Either way - I've pushed out this change right now.

@thysultan
Copy link
Owner

LGTM

@Andarist Andarist changed the title Add a test case for width prefixer ending up in an infinite loop Fixed an issue with prefixer going into an infinite recursion Dec 1, 2020
@Andarist
Copy link
Collaborator Author

Andarist commented Dec 1, 2020

Given that the final change is really proposed by you - I'm merging this right away.

@Andarist Andarist merged commit 106f6f6 into master Dec 1, 2020
@Andarist Andarist deleted the infinite-recursion-test-case branch December 1, 2020 14:12
@thysultan
Copy link
Owner

Published v4.0.4 with these last 2 PR's.

Thanks

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.

None yet

3 participants