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

Restructure source so that individual functions can be imported w/o importing the entire source #81

Closed
bent0b0x opened this issue Jun 3, 2018 · 7 comments

Comments

@bent0b0x
Copy link

bent0b0x commented Jun 3, 2018

Right now, if you want to import just one function in bip39 you must import the entire bundle. This can add unnecessary code to already large apps, making them larger. I propose that each function be written in their own files so that they can be imported independently of one another.

My specific concerns here are the the unorm dependency and words list files, which are quite large and not required for all of these utility functions.

Note: I plan to PR this repo with this change, but am open to thoughts in the meantime :)

@bent0b0x bent0b0x changed the title Restructure source and build output so that individual functions can be imported w/o importing the entire source Restructure source so that individual functions can be imported w/o importing the entire source Jun 3, 2018
@dcousens
Copy link
Contributor

dcousens commented Jun 4, 2018

I don't hate the change, but, honestly the source is 159 lines.
What are you really trying to achieve here?

edit:

My specific concern here is the unorm dependency, which is quite large and not required for all of these utility functions.

Nevermind, OK, that is the goal.

@dcousens
Copy link
Contributor

dcousens commented Jun 4, 2018

@bent0b0x the only function that is exposed presently that could take advantage of this, is entropyToMnemonic...

@dcousens
Copy link
Contributor

dcousens commented Jun 4, 2018

ES6 exports/imports, combined with tree shaking would make this irrelevant.

I think I'd sooner recommend that path then splitting up the source code unnecessarily when it nearly fits on 1 page.

@bent0b0x
Copy link
Author

bent0b0x commented Jun 4, 2018

the only function that is exposed presently that could take advantage of this, is entropyToMnemonic

yes and in turn generateMnemonic also benefits from this.

Also it seems like most of the word lists are required by any of the exposed functions. These files are relatively large, so not being required to import all of them just to use one function would be a nice win as well.

FWIW, I think it is nice to have each function isolated in the source anyway purely for organizational purposes but that is just me trying to be convincing that this change is worth merging 😄

@dcousens
Copy link
Contributor

dcousens commented Jun 4, 2018

Also it seems like most of the word lists are required by any of the exposed functions. These files are relatively large, so not being required to import all of them just to use one function would be a nice win as well.

This is addressed in #74

@dcousens
Copy link
Contributor

dcousens commented Jun 4, 2018

FWIW, I think it is nice to have each function isolated in the source anyway purely for organizational purposes but that is just me trying to be convincing that this change is worth merging smile

In an ideal world, everything might be a module. But in an ideal world, we might not even need to write code! Alas, I think breaking this file into mini-files of 10 lines hurts maintenance for something so small, therefore, NACK for that change alone.

@bent0b0x
Copy link
Author

bent0b0x commented Jun 5, 2018

That's great about version 3! Do you have an idea as to when it may be released?

Re: one file vs multiple files, I would argue that if someone had originally written this package w/ all functions in individual files I'm not sure anyone would have batted an eye. So migrating to that state to help consuming applications seems like an acceptable transition. Then again I do not maintain this package and I understand the resistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants