Skip to content

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

Merged
junedkazi merged 1 commit into
bigcommerce:masterfrom
jairo-bc:STRF-8607
Sep 22, 2020
Merged

refactor(storefront): STRF-8607 Update all dev dependecies on stencil utils#128
junedkazi merged 1 commit into
bigcommerce:masterfrom
jairo-bc:STRF-8607

Conversation

@jairo-bc

Copy link
Copy Markdown
Contributor

No description provided.

@bigbot

bigbot commented Aug 28, 2020

Copy link
Copy Markdown

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
Comment thread package.json Outdated
Comment thread package.json Outdated
Comment thread jest.config.js
@jairo-bc jairo-bc force-pushed the STRF-8607 branch 2 times, most recently from 832ee28 to 54a4ce5 Compare September 9, 2020 11:00
Comment thread babel.config.js Outdated
@jairo-bc jairo-bc force-pushed the STRF-8607 branch 2 times, most recently from 8375078 to 686904e Compare September 9, 2020 14:37
Comment thread babel.config.js Outdated
[
'@babel/preset-env',
{
targets: '> 1%, last 2 versions, Firefox ESR',

Copy link
Copy Markdown
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment thread package.json Outdated
"eventemitter3": "^4.0.4",
"whatwg-fetch": "^3.4.0"
},
"browserslist": "> 0.25%, not dead",

Copy link
Copy Markdown
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

Copy link
Copy Markdown

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 )

Comment thread webpack.conf.js
optimization: {
minimize: true,
},
devtool: false,

Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread .browserslistrc Outdated
@@ -0,0 +1 @@
last 1 version, >2% No newline at end of file

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread package.json
"eslint-plugin-import": "^2.22.0",
"jest": "^26.4.2",
"webpack": "^4.44.1",
"webpack-cli": "^3.3.12"

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread .browserslistrc Outdated
@@ -0,0 +1 @@
last 1 version, >2% No newline at end of file

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@junedkazi junedkazi left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

@junedkazi junedkazi left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

@junedkazi junedkazi left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread .browserslistrc Outdated
@@ -0,0 +1 @@
>2%, last 1 version, Firefox ESR No newline at end of file

Copy link
Copy Markdown
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