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

Add Babel setup #188

Merged
merged 1 commit into from
Jun 25, 2018
Merged

Add Babel setup #188

merged 1 commit into from
Jun 25, 2018

Conversation

pmuens
Copy link
Contributor

@pmuens pmuens commented May 3, 2018

What has been implemented?

Adds a Babel setup for the components core and all components in the registry. Support is added for Node versions as early as 4. Travis and AppVeyor build matrices were updated as well.

Closes SC-16

Steps to verify

Run npm run build and / or npm run watch.

Todos:

  • Write / update documentation
  • Run Prettier

@brianneisler
Copy link
Contributor

@pmuens, I wonder if it would be more maintainable to have one higher level build setup for all of the components instead of a separate one for each. Each component needs its own package.json to include its own dependencies during installation but i'm not sure if each one needs a separate build setup. Any thoughts on when we might need a setup like that?

@pmuens
Copy link
Contributor Author

pmuens commented May 4, 2018

@pmuens, I wonder if it would be more maintainable to have one higher level build setup for all of the components instead of a separate one for each. Each component needs its own package.json to include its own dependencies during installation but i'm not sure if each one needs a separate build setup. Any thoughts on when we might need a setup like that?

Thank you for your review @brianneisler 👍

Yes, I actually wrote a bash script while working on this to add the scripts and the dependencies automatically since doing this manually for every component would've been a real pain.

I thought along those lines but after thinking more about it I came up with the conclusion that every component should be self-contained since they might reside in other locations later on. Maybe that's a premature optimiation though. I'm open to other thoughts as well.

@pmuens pmuens force-pushed the add-babel-setup branch 25 times, most recently from ecdf36c to 6e3e26a Compare May 8, 2018 11:41
@pmuens pmuens force-pushed the add-babel-setup branch 12 times, most recently from ffaf292 to af5d7d4 Compare June 13, 2018 11:00
@pmuens pmuens removed the request for review from DavidWells June 13, 2018 11:01
@pmuens
Copy link
Contributor Author

pmuens commented Jun 13, 2018

After spending a good portion of time on this PR we can finally see that all builds for all the supported Node.js versions (4 - 10) are finally green on Travis and AppVeyor.

This PR can be considered done and needs a final review so that we can merge it :shipit:

/cc @eahefnawy @brianneisler

@pmuens pmuens dismissed brianneisler’s stale review June 13, 2018 11:07

Changes addressed

@eahefnawy
Copy link
Member

imo we should spend some time manually testing this PR in different node environments before merging. Travis can sometimes be misleading.

@ganeshvlrk
Copy link
Contributor

@pmuens Is there a specific reason we are holding off on merging this PR? Let me know.

@ganeshvlrk ganeshvlrk closed this Jun 21, 2018
@ganeshvlrk ganeshvlrk reopened this Jun 21, 2018
@pmuens
Copy link
Contributor Author

pmuens commented Jun 21, 2018

@pmuens Is there a specific reason we are holding off on merging this PR? Let me know.

@ganeshvlrk no. No specific reason. I haven't tested the support for all the different Node.js versions (this would be a daunting task). IMHO seeing that the tests pass should be enough for now...

@ganeshvlrk
Copy link
Contributor

Agreed. Let's go ahead and merge this on the basis of the tests passing

@pmuens
Copy link
Contributor Author

pmuens commented Jun 22, 2018

Agreed. Let's go ahead and merge this on the basis of the tests passing

@ganeshvlrk sounds good 👍

Can I just need an approval and we should be ready to merge this /cc @eahefnawy @brianneisler

eahefnawy
eahefnawy previously approved these changes Jun 22, 2018
@pmuens
Copy link
Contributor Author

pmuens commented Jun 25, 2018

Just tested this today running it in a fresh Node 4 Docker container.

Had to make some slight changes (e.g. Babel should be added to dependencies rather than devDependencies so that it's available when the package is installed globally). After that everything worked like a charm. Would like to merge it after the build passes 🚢 :shipit:

@pmuens pmuens merged commit 7d42088 into master Jun 25, 2018
@pmuens pmuens deleted the add-babel-setup branch June 25, 2018 11:02
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