Skip to content

Remove unused deps, add stand-alone polyfill for weakmap#9

Merged
elierotenberg merged 2 commits into
elierotenberg:masterfrom
bencripps:dependencyChanges
Nov 6, 2018
Merged

Remove unused deps, add stand-alone polyfill for weakmap#9
elierotenberg merged 2 commits into
elierotenberg:masterfrom
bencripps:dependencyChanges

Conversation

@bencripps

@bencripps bencripps commented Oct 28, 2018

Copy link
Copy Markdown
Collaborator

Background

So I'd really like to use this package, but after bundling it with my code I realized that this library brings an additional ~55k gzipped, which is just a non-starter for my project because our static asset size is really important to our user experience.

I took a look at the dependencies and I've made some changes:.

Changes

  1. I've removed Bluebird since it brings an additional ~21k (gzipped) with it. It actually looks like Bluebird was only used during test, and I don't think the runtime of the library actually depends on Promises. If it does (and I've missed that) I think it's totally reasonable to state in the docs, this library requires promises, and all users are expected to bring them in however they like (polyfill, etc).
  2. I've also removed lodash, since I can't actually find any uses of it in src or test.
  3. I've removed babel-polyfill, since it looks like the only reason it was added was to support WeakMaps. I understand the importance of WeakMap to the implementation, so rather than remove it entirely, I've removed babel-polyfill in favor of a polyfill that just brings in WeakMap. This polyfill is 700 bytes, while the babel-polyfill library is about 87kb (~29kb gzipped).
  4. I've added an .nvmrc file stating the library is built using Node 8 (this will ensure promises are available for test, and will have no impact on the runtime of the library, since we're transpiling anyway).
  5. I've added a .babelrc with the babel-preset-es2015 preset, to ensure we're transpiling the library down to support browsers that dont fully support ES2016. I've also added a "build" command which generates the dist

After these changes, I copied the built code into a project that uses react-traverse, and it still works as expected (hooray!), and I've also updated the tests config so the tests all pass as well.

Conclusion

These changes will bring the package size down from 55k (gzipped) to a about 800 bytes. I know this is a lot of changes, but almost all of these changes (except the weakmap replacement) have very little to do with the runtime of the library. Please let me know if you have any concerns!

@bencripps

Copy link
Copy Markdown
Collaborator Author

P.S. I reviewed the changes in #5 before making this PR, and although these PR's have overlap, I don't think these are super related.

Although this ticket simplifies the babel-config (somewhat), the main goal of this PR is to reduce the overall weight of the package.

@bencripps bencripps changed the title Remove unused deps, update babel-polyfill to suppoer weakmap Remove unused deps, add stand-alone polyfill for weakmap Oct 29, 2018
@bencripps

Copy link
Copy Markdown
Collaborator Author

Please let me know if you have questions, or if you won't have time to merge these -- if that's the case, I'm happy to use my own fork for now.

However, I think these changes greatly improve the library, and will make it easier for other developers to pick up as well.

@elierotenberg elierotenberg merged commit 272173e into elierotenberg:master Nov 6, 2018
@elierotenberg

Copy link
Copy Markdown
Owner

Many thanks for all your contributions!
Sorry I took so long to merge your PRs. If you're interested in helping me maintain this package, I'd be very glad. Feel free to DM me on twitter / direct mail :)

@bencripps

Copy link
Copy Markdown
Collaborator Author

That would be great!

@bencripps

Copy link
Copy Markdown
Collaborator Author

Would you mind publishing the latest changes that have been merged to master?

Although we haven't changed the public API, we have removed a bunch of the existing dependencies which could cause issues for users of this library, if they were relying on them transitively.

We've also changed the dist path, which is technically not a breaking change, but if someone was relying on a file at file path, this could break them as well.

It might be safer to publish this as a major instead of a minor.

@bencripps

Copy link
Copy Markdown
Collaborator Author

Additionally, if you can grant me access, I don't mind publishing the new version as well.

@bencripps

bencripps commented Dec 2, 2018

Copy link
Copy Markdown
Collaborator Author

It's been about a month since these changes were merged; any chance we can get them published, otherwise I'll have to publish a version of my fork.

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.

3 participants