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

Feature: Add babel transpiling to be backwards compatible with older Node.js versions #2

Merged
merged 7 commits into from
Oct 16, 2017

Conversation

bluzi
Copy link
Contributor

@bluzi bluzi commented Oct 12, 2017

Hi,

As we discussed, this PR adds Babel transpilation to the project.
From now on, you're going to have to run npm run build before you deploy a new version to npm. This will create a backward-compatible version of index.js in dist/.

Before merging this, there's something we still need to figure out together:
The listing files fail if the filespattern is empty test fails when executed against the transpiled code. I think it's because Babel ignores bluebird and uses its own Promise, but I'm not completely sure.
Anyway, I thought it will be best it I'll show it to you and make sure it's alright.

This is a complete list of what this PR adds:

  • Few Babel dependencies to devDependencies (babel-cli, babel-core, babel-polyfill, babel-preset-env)
  • .babelrc file
  • The source file has moved to src/ (just to separate it completely from dist/)
  • dist/ added to .gitignore
  • I added build script to package.json

I've tested it using both Node v4.8.4 and Node v8.6.0 using this code, and it seems to work flawlessly.

LMK if you want any changes.

@lirantal
Copy link
Owner

Thanks for jumping on this. I appreciate the elaborate explanation and the work on the PR!

Remaining items for us to sort out:

  1. If we need to ship a transpiled version for older Node.js then we need to ship dist/ with the npm package and remove it from git ignore, right? (or were you think we build it again on when it gets installed in the user's end?)
  2. As you mentioned, we indeed need to remember running the build script before publishing, but thanks to npm elaborate life-cycle scripts we can use the prepublish run script to do that (see npm-scripts). Since we're testing with the transpiled code as the target, it also makes sense that before tests are run we build, so it requires updating the test script to also build before executing tests.
  3. The problem is actually with the tests code itself. In const filesObj = myLicenses.fetch() filesObj is actually a promise object and we really need to async/await the result for myLicenses.fetch() because we're immediately checking the returned object. The pattern of not awaiting it worked in other tests because we were hooking into the events. In summary, the required change to fix it is really to update the function signature to be async and change that line to read const filesObj = await myLicenses.fetch()
  4. Lastly, since we're using standardjs, can you please update /src/index.js to remove the semicolon from the require at the top?

Great job! I think that's mostly it.
Could you make the above changes on the PR?

@bluzi
Copy link
Contributor Author

bluzi commented Oct 14, 2017

Hi @lirantal, it was my pleasure :)
Thanks for your comments, I fixed everything and now all the tests pass.

Travis still seems to be unhappy, but I don't see why. Any idea?

Copy link
Owner

@lirantal lirantal left a comment

Choose a reason for hiding this comment

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

great stuff!
minor change before we merge

package.json Outdated
"test": "node --harmony node_modules/.bin/ava",
"test:watch": "node --harmony node_modules/.bin/ava --watch",
"test:coverage": "nyc node --harmony node_modules/.bin/ava",
"test": "npm run build; node --harmony node_modules/.bin/ava",
Copy link
Owner

Choose a reason for hiding this comment

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

@bluzi we should use && between the commands instead of ; so that the 2nd part is only run if the first part is successful. can you make the change across the other commands as well? (test:watch, etc)

@bluzi
Copy link
Contributor Author

bluzi commented Oct 16, 2017

Done

LMK if there's anything else :)

@lirantal
Copy link
Owner

One last change - since you moved the index.js file and we're also babeling it, then the current pointer to the root index.js file in package.json's main field needs to be updated.

@lirantal
Copy link
Owner

To be clear and elaborate on the above comment, you need to make the following change in package.json: "main": "dist/index.js"

@bluzi
Copy link
Contributor Author

bluzi commented Oct 16, 2017

Yup, I understood :) one sec

@lirantal
Copy link
Owner

Cool. Sorry for the churn, I noticed it just now :)

@bluzi
Copy link
Contributor Author

bluzi commented Oct 16, 2017

np :)

@lirantal
Copy link
Owner

Amazing job! 💪
Thanks for being so patient and prompt and of course for contributing this!

@lirantal lirantal merged commit c5351d4 into lirantal:master Oct 16, 2017
@lirantal
Copy link
Owner

@bluzi some open items we have for additional work:

  1. Update the project with a proper travis ci file to run the tests (this is a good place to also check several Node.js versions so we can track when it breaks)
  2. Updating the README for proper docs on the supported Node.js versions
  3. There's a sister CLI project called https://travis-ci.org/lirantal/licenseye if you want to make the same babel support

I can open separate issues for each so we can tackle them in parallel PRs.

@bluzi
Copy link
Contributor Author

bluzi commented Oct 16, 2017

Sure, open an issue for everything you want to add, and I might take one or two :)
Consider adding the #hacktoberfest label. It's a good way to attract new contributors nowadays.

Anyway, that was fun and productive :)

@lirantal
Copy link
Owner

oh right! great idea, I'll adopt it here and in my other repos too.

@lirantal lirantal changed the title Babel Feature: Add babel transpiling to be backwards compatible with older Node.js versions Oct 16, 2017
@bluzi
Copy link
Contributor Author

bluzi commented Oct 16, 2017

Prepare to be overwhelmed with PRs :P

@lirantal
Copy link
Owner

Wouldn't take it any other way :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants