Small lints #3244
Small lints #3244
Conversation
🦋 Changeset detectedLatest commit: 1fa3297 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/utils/src/index.ts
Outdated
| registeredStyles.push(`${registered[className]};`) | ||
| } else { | ||
| registeredStyles.push(registered[className] as string) |
There was a problem hiding this comment.
registered contains serialized styles that are already terminated by a ; by the serialization code. It avoids allocating a new string.
There was a problem hiding this comment.
This was added in #1656 . Maybe it's good to consider that a mistake in hindsight but it doesn't give me confidence that we can remove this safely now. I'm surprised that tests are OK with this change (well, some failed but I'm not sure if this is related, I see some hashes changed but I'd expect some functional break too). Maybe we are missing a test case?
There was a problem hiding this comment.
The ; should definitely be changing the hash of strings passing through here, so that's expected.
The ; feels overly defensive to me and I don't like that it's also creating more strings for the same reasons explained in the other comment. It would be nice to ensure that what goes in cache.registered is strictly ; terminated, then we can safely do the change here and avoid modifying the strings everytime they pass here.
I'll revert it for now though, might require more investigation.
There was a problem hiding this comment.
For more context, I frequently see strings like this one:
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| registeredStyles.push(`${registered[className]};`) | ||
| } else { | ||
| registeredStyles.push(registered[className] as string) | ||
| } else if (className) { |
There was a problem hiding this comment.
classNames could have multiple sequential spaces, in which case .split(' ') would produce empty strings.
There was a problem hiding this comment.
a test case probably could be written for this, does it create a problem though? a rawClassName has already been allocated, can't this use some kind of string ropes or something to stay optimized?
There was a problem hiding this comment.
Yeah someone might have inserted additional whitespaces in their classnames.
So sure engines will usually use a concatenated string like I describe here to optimize concat operations, but adding a single ' ' is likely too small to be worth it, engines are most likely just allocating a new string for those bytes.
I wrote a benchmark with only this difference, it does seem to have a positive impact: (I inserted a bunch of spaces to highlight the impact more clearly)
| let labelPattern = /label:\s*([^\s;\n{]+)\s*(;|$)/g | ||
| let labelPattern = /label:\s*([^\s;{]+)\s*(;|$)/g |
There was a problem hiding this comment.
\s contains \n. See node -p "'\\n'.match(/\\s/)"
|
Could you add short changesets for this? Otherwise, those changes won't get released until some other PR mentions the touched packages in its changesets. The text that you put there will be automatically copied over to |
|
I have added the changeset, please let me know if there's anything else to do on my side. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
Various minor lints, see comments below.