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

feat: reduce bundle size by 1/3 #49

Merged
merged 8 commits into from
Dec 5, 2019
Merged

feat: reduce bundle size by 1/3 #49

merged 8 commits into from
Dec 5, 2019

Conversation

mikeal
Copy link
Contributor

@mikeal mikeal commented Sep 18, 2019

I did a few things here;

  1. replaced the Python script that generated 3 files with a Node.js script that generates only one file.
    • the module it outputs is also a bit different in order to reduce the size of the file more, even after minification/compression.
  2. replaces 2 codegen files with files that create their tables from the other generated file. this means a much smaller bundle and a negligible increase in import time.
  3. updates aegir and fixes new lint issues. i would have made this another PR but the version that was here was before aegir lint —fix was implemented so in order to get the linter passing I need to upgrade it.

@mikeal
Copy link
Contributor Author

mikeal commented Sep 18, 2019

the build is mad about the commit message, are we actually trying to be this strict about commit messages?

@vmx
Copy link
Member

vmx commented Sep 19, 2019

are we actually trying to be this strict about commit messages?

I think especially this case makes sense as there's tooling around the changelog, so it makes sense to have a fixed set of categories.

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

I'd like to get #51 merged before this PR as I added a test that fails with this PR :)

@hacdias
Copy link
Member

hacdias commented Sep 29, 2019

@mikeal can you rebase and check the build with the new tests, please? :)

@mikeal
Copy link
Contributor Author

mikeal commented Sep 30, 2019

My plate is pretty full for the next month or so, maybe @vmx can take this over if he has some cycles.

@hacdias
Copy link
Member

hacdias commented Oct 5, 2019

I can take a look at it myself!

mikeal and others added 2 commits October 5, 2019 20:40
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Oct 5, 2019

@vmx I fixed the test by only the adding the first element to the print table. In other words, since ipfs appears after p2p on the original table, it was getting overridden. The issue with this is: order matters. But I guess it already mattered on master.

@hacdias hacdias requested a review from vmx October 5, 2019 20:24
src/print.js Outdated Show resolved Hide resolved
src/print.js Outdated Show resolved Hide resolved
src/print.js Outdated Show resolved Hide resolved
src/print.js Outdated Show resolved Hide resolved
tools/update-table.js Outdated Show resolved Hide resolved
tools/update-table.js Outdated Show resolved Hide resolved
tools/update-table.js Outdated Show resolved Hide resolved
src/constants.js Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
@vmx
Copy link
Member

vmx commented Oct 7, 2019

The issue with this is: order matters. But I guess it already mattered on master.

Yes it was like this in master. The important part here is that we prefer p2p over ipfs, which was the reason why I added the test.

@hacdias
Copy link
Member

hacdias commented Oct 13, 2019

@vmx just went through the code and made some simplifications. Also, accepted your suggestion of saving the table to a .json file instead!

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

@hacdias Nice! I actually haven't thought of actually storing it as a JSON file. I just thought, using JSON somehow might be an idea, but that's now even better!

@mikeal As things changes quite a bit from your code, would you mind having another quick look over this PR (or please leave a comment if you can't be bothered :)

@alanshaw
Copy link
Member

Anything blocking this from getting merged and released?

@hacdias
Copy link
Member

hacdias commented Nov 22, 2019

@alanshaw we were just waiting for @mikeal review. Other than that, nothing! Wdyt?

@alanshaw
Copy link
Member

alanshaw commented Dec 4, 2019

@hacdias I think that this is soon to be the biggest module in ipfs-http-client and I'd like to reduce it by 1/3.

Screenshot 2019-12-04 at 17 16 51

As the lead maintainer of this module you should feel empowered to take ownership of this PR and merge it if you see fit 😁

If it were me, I'd double check the tests are passing, that the size reduction is still as anticipated and link it in to js-ipfs and run the tests to ensure nothing broke.

@hacdias hacdias merged commit 2422b09 into master Dec 5, 2019
@hacdias hacdias deleted the reduce-bundle branch December 5, 2019 16:04
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.

4 participants