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

refactor(storefront): STRF-8607 Update all dev dependecies on stencil utils #128

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

jairo-bc
Copy link
Contributor

No description provided.

@bigbot
Copy link

bigbot commented Aug 28, 2020

Autotagging @bigcommerce/storefront-team

@jairo-bc jairo-bc force-pushed the STRF-8607 branch 2 times, most recently from d7623b8 to 3d8ced2 Compare September 3, 2020 16:55
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jairo-bc jairo-bc force-pushed the STRF-8607 branch 2 times, most recently from 832ee28 to 54a4ce5 Compare September 9, 2020 11:00
babel.config.js Outdated Show resolved Hide resolved
babel.config.js Outdated
[
'@babel/preset-env',
{
targets: '> 1%, last 2 versions, Firefox ESR',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s make sure this matches the setting we have defined in cornerstone

Choose a reason for hiding this comment

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

package.json Outdated
},
"dependencies": {
"eventemitter3": "^4.0.4",
"whatwg-fetch": "^3.4.0"
},
"browserslist": "> 0.25%, not dead",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make this consistent with Babel target and also make sure it aligns with cornerstone config

Choose a reason for hiding this comment

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

We can even move it out to a separate .browserslistrc file so that this config is used by all the libraries which may need it (as it's recommended here: https://babeljs.io/docs/en/babel-preset-env#browserslist-integration )

optimization: {
minimize: true,
},
devtool: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we are changing this to false ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

devtool is used only in mode: 'development'. We are using 'production' build, so we don't need to specify it

@jairo-bc jairo-bc force-pushed the STRF-8607 branch 3 times, most recently from 9606836 to 3fdab62 Compare September 10, 2020 16:19
.browserslistrc Outdated
@@ -0,0 +1 @@
last 1 version, >2%
Copy link
Contributor

Choose a reason for hiding this comment

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

well I wonder if we need to define one here or should we just rely on what the theme has ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be used by babel, that is used in webpack and is in current repo(and other tools, that have possibility to use .browserslistrc https://github.com/browserslist/browserslist#browserslist-). We will need to sync all repos together, I've got a PR for cornerstone with more info bigcommerce/cornerstone#1836

"eslint-plugin-import": "^2.22.0",
"jest": "^26.4.2",
"webpack": "^4.44.1",
"webpack-cli": "^3.3.12"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need the cli package ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For running build from cli https://webpack.js.org/guides/installation/
Screenshot 2020-09-11 at 13 23 27

.browserslistrc Outdated
@@ -0,0 +1 @@
last 1 version, >2%
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why we are dropping firefox esr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 wonder if we should drop the lock file as well here ? @MaxGenash @jairo-bc @mattolson thoughts ?

@jairo-bc
Copy link
Contributor Author

I wonder if we should drop the lock file as well here ? @MaxGenash @jairo-bc @mattolson thoughts ?

Yep, makes sense, since it's a dependency library

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.

LGTM 👍 . I was wondering if you want to drop the package lock in this PR itself or in a follow up PR ?

@jairo-bc
Copy link
Contributor Author

LGTM 👍 . I was wondering if you want to drop the package lock in this PR itself or in a follow up PR ?

Just added to this one

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.

We should hold off on merging this one until we get a confirmation on .browserslistrc change from @bookernath and @bc-as

.browserslistrc Outdated
@@ -0,0 +1 @@
>2%, last 1 version, Firefox ESR
Copy link
Contributor

Choose a reason for hiding this comment

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

@jairo-bc this should be last 2 version

@junedkazi junedkazi merged commit 2a040ae into bigcommerce:master Sep 22, 2020
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