-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
the build is mad about the commit message, 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. |
There was a problem hiding this 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 :)
@mikeal can you rebase and check the build with the new tests, please? :) |
My plate is pretty full for the next month or so, maybe @vmx can take this over if he has some cycles. |
I can take a look at it myself! |
License: MIT Signed-off-by: Henrique Dias <[email protected]>
License: MIT Signed-off-by: Henrique Dias <[email protected]>
License: MIT Signed-off-by: Henrique Dias <[email protected]>
License: MIT Signed-off-by: Henrique Dias <[email protected]>
License: MIT Signed-off-by: Henrique Dias <[email protected]>
@vmx I fixed the test by only the adding the first element to the |
Yes it was like this in master. The important part here is that we prefer |
@vmx just went through the code and made some simplifications. Also, accepted your suggestion of saving the table to a |
ca1ed87
to
620c26c
Compare
There was a problem hiding this 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 :)
Anything blocking this from getting merged and released? |
@hacdias I think that this is soon to be the biggest module in 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. |
I did a few things here;
aegir lint —fix
was implemented so in order to get the linter passing I need to upgrade it.