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

STENCIL-3672 - Upgrade dependencies #1069

Merged
merged 1 commit into from
Oct 9, 2017
Merged

STENCIL-3672 - Upgrade dependencies #1069

merged 1 commit into from
Oct 9, 2017

Conversation

mjschock
Copy link
Contributor

What?

  • upgrades many of the dependencies (jquery and foundation major bumps have been left out, for example)

Tickets / Documentation

cc @bigcommerce/stencil-team

@@ -9,6 +9,22 @@
"no-alert": 0,
"consistent-return": 0,
"max-len": 0,
"import/first": 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these new additions are here so that we don't need to fix all these lint issues from the new version of airbnb's linter which marks them as errors by default

Copy link
Contributor

Choose a reason for hiding this comment

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

Before adding this, can you try using eslint --fix option? to see what can be automatically fixed

Copy link
Contributor Author

@mjschock mjschock Oct 9, 2017

Choose a reason for hiding this comment

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

@mcampa i ran through and tried fix after removing each of these one-by-one and was able to auto-fix a few of them. we could create tickets or issues to go in and correct the others

@@ -156,7 +156,7 @@ class FacetedSearch {
const id = $navList.attr('id');

// Toggle depending on `collapsed` flag
if (_.contains(this.collapsedFacetItems, id)) {
if (_.includes(this.collapsedFacetItems, id)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lodash got rid of _.contains in favor of _.includes

@@ -5,58 +5,60 @@
"private": true,
"author": "BigCommerce",
"license": "MIT",
"dependencies": {
"@bigcommerce/stencil-utils": "^1.0.7",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

webpack now complains about all of these deps being in devDeps and says to move them here

package.json Outdated
"grunt-karma": "^0.12.2",
"eslint": "^4.5.0",
"eslint-config-airbnb": "^15.1.0",
"eslint-plugin-react": "^7.3.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

while we're not using react, the airbnb package requires it as a peer dep

Copy link
Contributor

@mcampa mcampa left a comment

Choose a reason for hiding this comment

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

LGTM 👍

CHANGELOG.md Outdated
@@ -6,6 +6,7 @@
- Do not scale product thumbnail images [#1094](https://github.com/bigcommerce/cornerstone/pull/1094)
- Lazy load carousel images [#1090](https://github.com/bigcommerce/cornerstone/pull/1090)
- Theme Editor menu item updates for ease of use [#1091] (https://github.com/bigcommerce/cornerstone/pull/1091)
- Upgrades all dependencies except for Foundation and jQuery [#1069](https://github.com/bigcommerce/cornerstone/pull/1069)
Copy link
Contributor

Choose a reason for hiding this comment

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

@mjschock can you fix the line above & remove the space between [#1091] (https://github.com/bigcommerce/cornerstone/pull/1091) 🙏

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 👍

"lazysizes": "3.0.0",
"lodash": "^4.17.4",
"nod-validate": "^2.0.12",
"pace": "hubspot/pace#a03f1f1de62c9cea6c88b2267b8d7a83858b6cb6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know why pace is locked down to a commit & not to an actual release ?

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 IIRC it's because they made a breaking change. i'll do a quick check to see if we can update now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, there's a breaking change. let's just create a separate PR with the fix

Copy link
Contributor

@junedkazi junedkazi left a comment

Choose a reason for hiding this comment

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

I do see templates/components/common/icons/icon-defs.html generated again. Is it supposed to ?

@mjschock
Copy link
Contributor Author

mjschock commented Oct 9, 2017

@junedkazi regarding the icon-defs, yeah i believe it should change this time since grunt-svgstore has been updated. i just checked out the version from master and ran grunt and it gets updated

@mjschock mjschock merged commit cf348aa into bigcommerce:master Oct 9, 2017
@mjschock mjschock deleted the STENCIL-3672 branch October 9, 2017 20:48
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.

None yet

3 participants