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

Switched to babel ES5 build in dist folder for package distribution #37

Merged
merged 1 commit into from
Mar 7, 2018

Conversation

holgerd77
Copy link
Member

Since topic also came up here:

This PR switches to babelified ES5 packaging. Tests are included in the babel build. For test run with the test script, distribution tests are used, coverage is using the original test files in the test folder.

Copy link

@mohammadrafigh mohammadrafigh left a comment

Choose a reason for hiding this comment

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

great, thanks 👍 but why dist folder is ignored?

@holgerd77
Copy link
Member Author

Dist folder is only created for the build, otherwise there is the risk if inconsistent repository states.

@mohammadrafigh
Copy link

ok but this way npm won't fetch and build the dist folder when using npm install ethereumjs-wallet in another project. or maybe you want to publish dists separately?

@holgerd77
Copy link
Member Author

I have just tested this for another library (ethereumjs-util) to double-check, dist folder is actually included there when library is installed with npm.

The dist folder is included with the files directive in package.json, so it is distributed with the npm package, npm is ignoring .gitignore here. Do you have any other information beyond that I didn't consider here?

@mohammadrafigh
Copy link

I also use ethereumjs-util and it's ok, in my node_modules there is a dist folder. but there is no dist folder in ethereumjs-wallet when fetching from github using npm. maybe there is a bug in npm that won't run build script for dependencies fetched from git. so if this is the case then yes this PR is ok IMO. thanks for your fast PR.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.62% when pulling 99b5d3d on es5-dist into 01382b1 on master.

@holgerd77 holgerd77 merged commit 86464c9 into master Mar 7, 2018
@holgerd77 holgerd77 deleted the es5-dist branch March 7, 2018 09:56
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