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.

Sign up for GitHub

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

Update webpack environment targets #4649

Merged
merged 1 commit into from
Aug 3, 2023
Merged

Update webpack environment targets #4649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions .browserslistrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
[production]
last 2 Firefox versions
last 2 Chrome versions
last 2 Safari versions
> 0.25%
not ie 11
Firefox > 0 and last 2 years and > 0.01%
Chrome > 0 and last 2 years and > 0.01%
Safari > 0 and last 2 years and > 0.01%
Edge > 0 and last 1 years and > 0.01%
Opera > 0 and last 2 years and > 0.01%
> 0.2%
not op_mini all
not and_uc < 100
not android < 100
not dead

[dev]
last 1 chrome versions
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Removes `minimatch` manual resolution ([#3019](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3019))
- Upgrade `vega-lite` dependency from `4.17.0` to `^5.6.0` ([#3076](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3076)). Backwards-compatible version included in v2.5.0 release.
- Bump `js-yaml` from `3.14.0` to `4.1.0` ([#3770](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3770))
- Update webpack environment targets ([#4649](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4649))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Update webpack environment targets ([#4649](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4649))
- Update webpack environment targets to expand supported browser matrix and reduce polyfills and hacks ([#4649](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4649))


### 🪛 Refactoring

Expand Down
2 changes: 1 addition & 1 deletion packages/osd-babel-preset/node_preset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ module.exports = (_, options = {}) => {
// `nvm install 8 && node ./src/cli` will run OpenSearch Dashboards
// in node version 8 and babel will stop transpiling async/await
// because they are supported in the "current" version of node
node: 'current',
node: 14,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Left as current, we could be building with Node 18 and producing artifacts that are not compatible with Node 14, while claiming to allow users to use Node 14 with OSD. This makes sure any built and released artifact will work on Node 14.

Copy link
Member

Choose a reason for hiding this comment

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

I figured as much, what i worry is that this means that we dont benefit from any of the node 18 updates just because we build on node 14. I think we should build by default on the latest version of node and anyone who wants to use an older version can but they need to rebuild the artifacts themselves. We shouldnt build for 14 by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we package the artifacts of a release, consumers of those artifacts don't have the option of rebuilding. Also, since this the CommonJS part of the code that runs on the server, payload size it not a worry.

},

// replaces `import "core-js/stable"` with a list of require statements
Expand Down
4 changes: 4 additions & 0 deletions packages/osd-babel-preset/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
"babel-plugin-add-module-exports": "^1.0.4",
"babel-plugin-styled-components": "^2.0.2",
"babel-plugin-transform-react-remove-prop-types": "^0.4.24",
"browserslist": "^4.21.10",
"react-is": "^16.8.0",
"styled-components": "^5.3.9"
},
"devDependencies": {
"@types/browserslist": "^4.15.0"
}
}
5 changes: 5 additions & 0 deletions packages/osd-babel-preset/webpack_preset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
* under the License.
*/

const { resolve } = require('path');
const browserlist = require('browserslist');
const targets = browserlist.loadConfig({ path: resolve(__dirname, '../..') });
Comment on lines +31 to +33
Copy link
Member

Choose a reason for hiding this comment

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

nice, thanks for this change!


module.exports = () => {
return {
presets: [
Expand All @@ -36,6 +40,7 @@ module.exports = () => {
{
useBuiltIns: 'entry',
modules: false,
targets,
// Please read the explanation for this
// in node_preset.js
corejs: '3.2.1',
Expand Down
4 changes: 3 additions & 1 deletion packages/osd-optimizer/postcss.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,7 @@
*/

module.exports = {
plugins: [require('autoprefixer')()],
plugins: [
/*require('autoprefixer')()*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will be raising a PR on autoprefixer to silent their warning when they are not needed via an option flag. They wrongly assume that not being needed today would make them not needed in the future.

Today, they generate a ton of warnings about being not needed, masking important log messages.

Copy link
Member

Choose a reason for hiding this comment

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

A little more context here would help, not a blocker but where are these warnings generated? Also cant we just wait for the PR that fixes auto prefixer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

autoprefixer is a PostCSS plugin belonging to the days when browsers has vendor prefixes for rules. The revised browser matrix makes multi-vendor prefixing for all the existing prefixes obsolete and autoprefixer throws a warning for every single time it is sees a prefix saying as much.

However, what autoprefixer fails to remember is that "existing" is not future-proof and any day, we could have a new rule created that is thrown behind vendor prefixes.

The change I will be proposing will allow consumers of autoprefixer to silent this warning using an option. Until that change is made, merged, and released, I am stopping the use of autoprefixer. However, I am not removing it completely because we would want it back as soon as we have an update.

Copy link
Member

Choose a reason for hiding this comment

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

I think the preferred approach for these cases is a TODO comment that links to an issue - it allows someone in the future to just remove the commented code if they decide this issue is no longer valid.

],
};
2 changes: 1 addition & 1 deletion packages/osd-optimizer/src/integration_tests/__snapshots__/basic_optimization.test.ts.snap
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This snapshot diff is pretty unintelligible to me, so I'm just assuming it's fine.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion tsconfig.browser.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"extends": "./tsconfig.base.json",
"compilerOptions": {
"target": "es5",
"target": "es2018",
Copy link
Member

Choose a reason for hiding this comment

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

What's the implication of this change?

"module": "esnext",
},
"include": [
Expand Down
Loading