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.

Sign up for GitHub

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 extra exports and use version from pkg.json #819

Merged
merged 1 commit into from
Oct 3, 2016
Merged

Remove extra exports and use version from pkg.json #819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions lib/chai.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
* MIT Licensed
*/

var used = []
, exports = module.exports = {};
var used = [];

/*!
* Chai version
*/

exports.version = '3.5.0';
exports.version = require('../package').version;
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI our tooling automates writing this out as the string, so that we don't end up including all of the package.json in the browserify bundle.

Copy link
Contributor

Choose a reason for hiding this comment

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

@keithamus Whoops. Revert? Also now that we have more maintainers/reviewers, maybe we should add a minimum time before merging a PR even after it has two approvals.

Copy link
Member

@vieiralucas vieiralucas Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

OH,

Should we revert this?

Sorry for merging, should've waited for you.

Copy link
Contributor Author

@shvaikalesh shvaikalesh Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, when bundling for browsers result package size is an issue. We can use Rollup to decrease bundle size (tree-shaking and stuff) and include only the version field of package.json.

Copy link
Member

Choose a reason for hiding this comment

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

@meeber @vieiralucas don't worry about reverting, our build process will write over this change when we release 4.0

@shvaikalesh We've already got the tooling that writes the version. We can definitely look at Rollup though; do you want to open an issue to discuss it?

Copy link
Member

Choose a reason for hiding this comment

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

Also @meeber - I don't think we should add things like minimum time to review. This stuff happens, it's teething issues, but ultimately no big deal. Nothing egregious has happened - when it does we'll deal with it then!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#821


/*!
* Assertion Error
Expand Down
6 changes: 0 additions & 6 deletions lib/chai/utils/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,6 @@
* MIT Licensed
*/

/*!
* Main exports
*/

var exports = module.exports = {};

/*!
* test utility
*/
Expand Down