search

ethereumjs / keythereum

Create, import and export Ethereum keys
MIT License
609 stars 163 forks source link

Remove dist folder #22

Closed fanatid closed 7 years ago

fanatid commented 7 years ago

build-jslib-ci is experimental project (keyethereum is a first project who will use it 😄) keythereum.js and keythereum.min.js will be uploaded to github releases on every version bump 🚀

tinybike commented 7 years ago

I'm not sure I understand what build-jslib-ci does. It auto-creates a dist/ folder every time the version is bumped? Is this auto-created dist/ folder on npm as well as github?

fanatid commented 7 years ago

build-jslib-ci auto-create dist folder on CI server on every version bump and upload files from there to github release It works like prebuild-ci which generate bindings and upload them to github: https://github.com/Level/leveldown/releases/tag/v1.6.0

Is this auto-created dist/ folder on npm as well as github?

No. Why you need dist/ in npm package?

fanatid commented 7 years ago

any comments?

tinybike commented 7 years ago

I like the idea! But, I think I would prefer to keep the dist/ folder in the repo for now. I like having the dist folder there because it makes using keythereum in the browser absolutely dead simple. It's becoming less common to directly import JS files via script tag, but when script tags are used, they tend to be used by less experienced developers, so I like to keep things as simple for them as possible.

fanatid commented 7 years ago

I still don't understand motivation... files from dist will be uploaded to github releases and will be available like files from repository and why need dist in npm?

tinybike commented 7 years ago

If the dist files are only uploaded to releases, then someone checking out the repo will not see them unless they checkout a release. (Or, am I misunderstanding how build-jslib-ci works?) It is not a big deal, but I think people may not realize you have to manually checkout a release to get the pre-browserified distributable.

You're correct that dist is not needed in npm. I should have excluded it from npm but had not done so. I've just now added dist to .npmignore. Good catch, thank you!

fanatid commented 7 years ago

If the dist files are only uploaded to releases, then someone checking out the repo will not see them unless they checkout a release. (Or, am I misunderstanding how build-jslib-ci works?) It is not a big deal, but I think people may not realize you have to manually checkout a release to get the pre-browserified distributable.

We can put notes about this to README. The main idea of build-jslib-ci that you don't need care that you really build files in dist for every release. Also when you merge PR dist is not updated automatically and if you look on files in commit https://github.com/ethereumjs/keythereum/tree/9ba61fc1f3df19109c98dffbf6f03d4dd52025c7 you can see that dist not match with code, but this is not really big deal.

You're correct that dist is not needed in npm. I should have excluded it from npm but had not done so. I've just now added dist to .npmignore.

Not big fun of .npmignore, I thunk files in package.json is better because it white list (let me know if you want change it, I'll create PR). JFYI some names will be always ignored/added: https://github.com/npm/npm/blob/v4.5.0/lib/utils/tar.js#L66

fanatid commented 7 years ago

I don't want force you accept this PR.. this is only my opinion.