Replace DevTools semver usages with compare-versions for smaller bundle size
#26122
Conversation
markerikson
force-pushed
the
feature/simplify-devtools-semver
branch
2 times, most recently
from
320e65d
to
2673489
Compare
|
|
||
| function gte(a: string = '', b: string = '') { | ||
| return semvercmp(a, b) > -1; | ||
| } |
There was a problem hiding this comment.
nit: These should probably go into a file like packages/react-devtools-shared/src/backend/utils.js since they're not specifically related to React (and that way we won't have to define them twice)
There was a problem hiding this comment.
Thanks for the pointer, done!
markerikson
force-pushed
the
feature/simplify-devtools-semver
branch
2 times, most recently
from
f3e1ac9
to
d1fd90e
Compare
| if (isNaN(na) && !isNaN(nb)) return -1; | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Could use a few basic unit tests :D
| const pb = b.split('.'); | ||
| for (let i = 0; i < 3; i++) { | ||
| const na = +pa[i]; | ||
| const nb = +pb[i]; |
There was a problem hiding this comment.
This function assumes all characters in the string are numbers after split by '.'. However in renderer.js there a usage of gte(version, '18.0.0-alpha')
There was a problem hiding this comment.
Hmm. Good point. This appears to convert to NaN.
I'd have to think about the intended semantics here a bit more to decide on what an appropriate numeric conversion and comparison would be. Let me get back to you on that one.
There was a problem hiding this comment.
I found this https://semver.org/#spec-item-11
Hope it's helpful!
BTW I'm fine with a simplified compare logic as long as it works with React version numbers
There was a problem hiding this comment.
So it turns out that the NaN behavior here actually works as desired.
I pulled in the test code from https://unpkg.com/browse/[email protected]/test/cmp.js and expanded it with several pre-release examples. Those do actually sort correctly, like:
expect(sortedVersions).toEqual([
'1.2.3',
'1.5.5',
'1.5.19',
'2.3.1',
'4.1.3',
'4.2.0',
'4.11.6',
'10.5.5',
// Specifically check non-numerical pre-release versions
'11.0.0-alpha.0',
'11.0.0-alpha.1',
'11.0.0',
'11.3.0',
]);I also tweaked the logic to handle more dots. I don't think React uses that specific -alpha.N convention the way I do for Redux, but it was easy enough to add.
I think this resolves that issue.
There was a problem hiding this comment.
consider this case
const unsortedVersions = ['18.0.2', '18.0.1', '18.0.1-alpha', '18.0.0-alpha.1', '18.0.2-alpha', '18.0.1-alpha.1', '18.0.0'];
unsortedVersions.slice().sort(semvercmp);
(7) ['18.0.1-alpha', '18.0.2-alpha', '18.0.0-alpha.1', '18.0.1-alpha.1', '18.0.0', '18.0.1', '18.0.2']There was a problem hiding this comment.
I did some additional searching on NPM. I'd originally been copying code from https://www.npmjs.com/package/semver-compare . After looking around, I found https://www.npmjs.com/package/compare-versions . It's a bit larger (~200 lines, but that includes lots of comments, and it's only 625b min+gz), but seems to 100% match results from using the actual semver library.
I can either add compare-versions as a dependency, or copy-paste and inline it with attribution as I was doing earlier. Is there a preference which we go with?
There was a problem hiding this comment.
I vote for adding compare-versions as a dependency.
Thanks for doing this!
There was a problem hiding this comment.
Okay, I'll go ahead and remove the inlined behavior and switch to compare-versions as an added dependency.
There was a problem hiding this comment.
Awesome! thank you all for sorting this all out 🙇🏼
markerikson
force-pushed
the
feature/simplify-devtools-semver
branch
from
d1fd90e
to
337ae03
Compare
markerikson
force-pushed
the
feature/simplify-devtools-semver
branch
from
337ae03
to
4dc651f
Compare
|
Comparing: f0cf832...4dc651f Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
Okay, I've updated this to use |
|
lgtm! |
markerikson
changed the title
semver usages with a simpler inlined methodsemver usages with compare-versions for smaller bundle size
…bundle size (#26122) <!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https:/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn debug-test --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https:/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary This PR: - Replaces the existing usages of methods from the `semver` library in the React DevTools source with an inlined version based on https://www.npmjs.com/package/semver-compare. This appears to drop the unminified bundle sizes of 3 separate `react-devtools-extensions` build artifacts by about 50K:  ## How did you test this change? I was originally working on [a fork of React DevTools](replayio#2) for use with https://replay.io , specifically our integration of the React DevTools UI to show the React component tree while users are debugging a recorded application. As part of the dev work on that fork, I wanted to shrink the bundle size of the extension's generated JS build artifacts. I noted that the official NPM `semver` library was taking up a noticeable chunk of space in the bundles, and saw that it's only being used in a handful of places to do some very simple version string comparisons. I was able to replace the `semver` imports and usages with a simple alternate comparison function, and confirmed via hands-on checks and console logging that the checks behaved the same way. Given that, I wanted to upstream this particular change to help shrink the real extension's bundle sizes. I know that it's an extension, so bundle size isn't _as_ critical a concern as it would be for a pure library. But, smaller download sizes do benefit all users, and that also includes sites like CodeSandbox and Replay that are using the React DevTools as a library as well. I'm happy to tweak this PR if necessary. Thanks! DiffTrain build for [78d2e9e](78d2e9e) [View git log for this commit](https:/facebook/react/commits/78d2e9e2a894a7ea9aa3f9faadfc4c6038e86a75)
Summary
This PR:
semverlibrary in the React DevTools source with the equivalent method from the https://www.npmjs.com/package/compare-versions libraryThis appears to drop the unminified bundle sizes of 3 separate
react-devtools-extensionsbuild artifacts by about 50K:How did you test this change?
I was originally working on a fork of React DevTools for use with https://replay.io , specifically our integration of the React DevTools UI to show the React component tree while users are debugging a recorded application.
As part of the dev work on that fork, I wanted to shrink the bundle size of the extension's generated JS build artifacts. I noted that the official NPM
semverlibrary was taking up a noticeable chunk of space in the bundles, and saw that it's only being used in a handful of places to do some very simple version string comparisons.I was able to replace the
semverimports and usages with a simple alternate comparison function, and confirmed via hands-on checks and console logging that the checks behaved the same way.Given that, I wanted to upstream this particular change to help shrink the real extension's bundle sizes.
I know that it's an extension, so bundle size isn't as critical a concern as it would be for a pure library. But, smaller download sizes do benefit all users, and that also includes sites like CodeSandbox and Replay that are using the React DevTools as a library as well.
I'm happy to tweak this PR if necessary. Thanks!