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

Adding Support for Node 10 #525

Merged
merged 14 commits into from
Oct 14, 2019
Merged

Adding Support for Node 10 #525

merged 14 commits into from
Oct 14, 2019

Conversation

ncheikh
Copy link
Contributor

@ncheikh ncheikh commented Oct 10, 2019

What?

Updating stencil styles dependency which allows node 10 support.

Tested

  • Locally against cornerstone & node 8
  • Locally against cornerstone & node 10

Copy link
Contributor

@bookernath bookernath left a comment

Choose a reason for hiding this comment

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

Changeset seems sensible. Let's do some manual testing!

@ncheikh ncheikh changed the title Adding Support for Node 10-12 Adding Support for Node 10-11 Oct 10, 2019
@ncheikh ncheikh changed the title Adding Support for Node 10-11 Adding Support for Node 10 Oct 10, 2019
@xzyfer
Copy link

xzyfer commented Oct 11, 2019

Waiting on: dlmanning/gulp-sass#748 for node 12

No need to wait. Node Sass already supports node 12.

@@ -7,6 +7,8 @@ addons:
- github.com
node_js:
- 8
- 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add 9 as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, odd versions are only supported for a short time and I don't think we need to support node versions that aren't supported for cli. In the future I can see doing the leading version like 13 for early adopters and it helps us make sure we are in a good position for the next even/lts release.

Thoughts?

@junedkazi @bookernath @bc-williamkwon

https://nodejs.org/en/about/releases/

@@ -4,7 +4,7 @@
"description": "CLI tool to run BigCommerce Stores locally for theme development.",
"main": "index.js",
"engines": {
"node": ">= 8.0.0 <9.0.0"
"node": ">= 8.0.0 <11.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Also can we also update the readme to reflect node version support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@@ -6,7 +6,7 @@
The BigCommerce server emulator for local theme development.

## Install
_Note: Stencil requires the Node.js runtime environment, version 8.x. We do not yet have support for Node 10 or greater._
_Note: Stencil requires the Node.js runtime environment, version 8.x or 10.x We do not yet have support for Node 12 or greater._

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junedkazi @bookernath Let me know if we want different messaging here.

@ncheikh ncheikh merged commit 3013fb4 into bigcommerce:master Oct 14, 2019
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