Skip to content

Commit

Permalink
WIP: sort values to match by length
Browse files Browse the repository at this point in the history 
So that values with the same prefixes don't stomp over each other. This feels a bit hacky but works for now.

Fixes #363
  • Loading branch information
@tivac
tivac committed Nov 10, 2017
1 parent a2f5f58 commit 2c66d62
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 20 deletions.
40 changes: 20 additions & 20 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/core/plugins/values-replace.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ module.exports = (css, result) => {

matchRegex = new RegExp(
Object.keys(values)
.sort((a, b) => b.length - a.length)

This comment has been minimized.

Sign in to view

Copy link
@pygy

pygy Nov 11, 2017

If it can help you feel better about the fix, that was my first idea as well. I think it is optimal as it guarantees that potential prefixes are replaced last, without resorting to string content comparisons that would be costlier.

This comment has been minimized.

Sign in to view

Copy link
@pygy

pygy Nov 11, 2017 •
edited
Loading

Looking at the broader context, you may get a speedier matcher by sorting first alphabetically then by length, allowing the regex engine to build an efficient trie (if it can do such a thing). Otherwise you could build a trie-like regex manually.

This comment has been minimized.

Sign in to view

Copy link
@pygy

pygy Nov 11, 2017 •
edited
Loading

I was offline this afternoon and whipped up this, only to find out that this lib existed. It has been vetted by Mathias Bynens, I'd rather go with it.

new RegExp('\b' + new RegExpTrie().add(Object.keys(values)).toString() + '\b') will give you an optimized matcher.

One caveat with \b: /\bx\b/ will match -x-, so there may be false positives, depending on what your values can contain.

You may want to go with '([^\w-]|^)(' + new RegExpTrie()... + ')([^\w-]|$), and restore the first/last char when returning.

This comment has been minimized.

Sign in to view

Copy link
@tivac

tivac Nov 12, 2017

Author Owner

regex-trie is an interesting approach, I wonder if there'd be much benefit to processing time.

I've never been thrilled about my chosen approach to replacing @value instances, it feels really brittle to me currently.

This comment has been minimized.

Sign in to view

Copy link
@pygy

pygy Nov 12, 2017 •
edited
Loading

What's the shape of the node.value fields that are to be replaced? Could one of these hold either a prefix or a suffix of one of the values items (node.value not being a values item)? If so, the use of \b is indeed problematic ('([^\w-]|^)(' + valuesMatcher + ')([^\w-]|$) being a solution).

Explicitly, if values is ['foo'], and node.value is 'hi-foo-bar', you'll get a false positive match.

Otherwise sorting the values by length before matching should be robust AFAICT.
.map((v) => `\\b${escape(v)}\\b`)
.join("|"),
"g"
Expand Down
4 changes: 4 additions & 0 deletions packages/core/test/__snapshots__/values.test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ exports[`/processor.js values should support value namespaces 1`] = `
.blue {
color: blue;
}
.other {
color: #000;
}
"
`;

Expand Down
4 changes: 4 additions & 0 deletions packages/core/test/specimens/value-namespace.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@
.blue {
color: colors.b;
}

.other {
color: colors.base-other;
}
2 changes: 2 additions & 0 deletions packages/core/test/specimens/values.css
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
@value a: red;
@value b: blue;
@value base: #FFF;
@value base-other: #000;

0 comments on commit 2c66d62

Please sign in to comment.