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

Remove Babel #1074

Merged
merged 6 commits into from
Jul 18, 2018
Merged

Remove Babel #1074

merged 6 commits into from
Jul 18, 2018

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented Jul 13, 2018

We no longer use import 🎉

Fixes #1062

@nventuro nventuro requested a review from frangio July 13, 2018 19:16
@shrugs
Copy link
Contributor

shrugs commented Jul 18, 2018

do we use jshint at all, actually? I don't think we do, so we can delete .jshintrc

@shrugs
Copy link
Contributor

shrugs commented Jul 18, 2018

LGTM after rebase @nventuro

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

I think the package-lock needs to be updated. Otherwise LGTM.

import { ethGetBalance, ethSendTransaction } from './helpers/web3';
const { ethGetBalance, ethSendTransaction } = require('./helpers/web3');

var SecureTargetBounty = artifacts.require('SecureTargetBounty');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use const for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we're only removing Babel in this PR, that's coming on a separate one to keep everything tidy.

Copy link
Contributor Author

@nventuro nventuro Jul 18, 2018

Choose a reason for hiding this comment

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

Opened #1091.

}
}
}
},
"babel-preset-env": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should've probably been removed, right? Re-run npm install?

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into that as well, but they're dev dependencies of solidity-coverage, so they're in our package-lock

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it. 🙂

@nventuro nventuro merged commit cea2a85 into OpenZeppelin:master Jul 18, 2018
@nventuro nventuro deleted the remove-babel branch July 18, 2018 22:37
@sweatyc
Copy link

sweatyc commented Jul 25, 2018

that would be nice, since I'm not a Node.js / web expert and currently scratching my head on trying to use the helper scripts for test but import and export don't work (due to ES6 environment I guess?)...

@nventuro
Copy link
Contributor Author

@sweatyc what do you mean? All helpers are imported using require now that this PR has been merged, a testing guide with information on how to use them, example, etc., is on the way (#1077).

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