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

Repair build scripts to output an es5-compatible module #18

Merged
merged 1 commit into from
Nov 9, 2018
Merged

Repair build scripts to output an es5-compatible module #18

merged 1 commit into from
Nov 9, 2018

Conversation

pgoldrbx
Copy link

@pgoldrbx pgoldrbx commented Nov 9, 2018

Description

I did not realize that the original maintainer had updated the repo to use es6 syntax but incorrectly implemented the build scripts. As a result, version 1.0.0 was not es5-compatible and this resulted in errors when used in webpack with uglify.

I've fixed the build scripts and did a bit of other cleanup.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore, documentation, cleanup

Related Issue

Version 1.0.0 was added to to jsonmltoreact in CondeNast/jsonmltoreact#16. This led to build errors when trying to update in https://github.com/conde-nast-international/voguede-autopilot/pull/3165

Motivation and Context

1.0.0 was an unintentionally breaking change due to syntax incompatibility.

How Has This Been Tested?

I have not added any additional testing to verify that babel works as expected. If desired, I could add a dist test that runs the build and then passes the result through uglify-js. However, I don't feel that's really necessary.

Screenshots (if appropriate):

n/a

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation (if required).
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@pgoldrbx pgoldrbx added the bug Something isn't working label Nov 9, 2018
@pgoldrbx pgoldrbx self-assigned this Nov 9, 2018
@pgoldrbx pgoldrbx requested review from giles-v and gazs November 9, 2018 17:07
/npm-debug.log
.nyc_output
coverage
dist
Copy link
Author

Choose a reason for hiding this comment

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

added dist and normalized line endings to LF

### Fixed
- Repair build scripts to output an es5-compatible module

## Version 1.0.0 - 2018-10-09
Copy link
Author

Choose a reason for hiding this comment

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

the 1.0.0 release wasn't added to the changelog. I'll clean this up further in a release commit.

const utils = require('./utils');
const dom = require('./dom');
const html = require('./html');
Object.assign(module.exports, utils, dom, html);
Copy link
Author

Choose a reason for hiding this comment

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

moved here from root directory, to ensure all code passes through babel

"files": [
"*.md",
"dist"
],
Copy link
Author

Choose a reason for hiding this comment

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

limit the files published with the package

@pgoldrbx pgoldrbx merged commit 05d9aad into CondeNast:master Nov 9, 2018
@pgoldrbx pgoldrbx deleted the fix/build-dist branch November 9, 2018 17:12
@coveralls
Copy link

Pull Request Test Coverage Report for Build 56

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 64.689%

Totals Coverage Status
Change from base Build 54: 0.0%
Covered Lines: 269
Relevant Lines: 375

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants