oui

oui copied to clipboard

Description

Improved version of https://github.com/opensearch-project/oui/pull/765. It would take a long time to address all the potential improvements, so I created a new PR where I applied them.

Run the following commands again to see if there are any errors:

• [X] yarn build
• [X] yarn build-docs
• [X] yarn start
• [X] yarn test-unit
• [X] yarn lint

Issues Resolved

https://github.com/opensearch-project/oui/issues/584

Check List

• [ ] New functionality includes testing.
• [ ] New functionality has been documented.
• [X] All tests pass
  • [X] yarn lint
  • [X] yarn test-unit
• [X] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Also https://github.com/opensearch-project/oui/commit/20869800a42d1f658b3a2d4bde4d9a7a4df13296 and https://github.com/opensearch-project/oui/commit/e9a63afd05e4bb532f30fd9359a71704e15a8ded need to be reverted, as we want to backport this to 1.x :)

Also 2086980 and e9a63af need to be reverted, as we want to backport this to 1.x :)

If I revet these commits, then my changes won't go into main. Should I create another PR where I just do the cherry-pick and the necessary changes for 1.x?

Created https://github.com/opensearch-project/oui/issues/810 for the reverting - @andreymyssak will make a separate PR for that. I've also assigned @BSFishy to this PR for Dashboards integration testing.

Summary of Work Done

Dependencies

• Bumped webpack from v4 to v5
• Bumped postcss from v7 to v8
• Bumped babel minor version
• Bumped typescript from v4.0.5 to v4.1.6 (https://github.com/opensearch-project/oui/pull/770#discussion_r1199626632)
• Removed some of the resolutions that were covered by a dependency update

Formatting

Prettier was not applied to some files in the script folder before, so the changes can be found in the following files (https://github.com/opensearch-project/oui/pull/770#discussion_r1199626994)

scripts/babel/fetch-i18n-strings.js
scripts/babel/react-docgen-typescript.js
scripts/eslint-plugin/forward_ref_display_name.js
scripts/eslint-plugin/i18n.js

Questions Requiring Further Discussion

1. Can we directly use port 8030 instead of utilizing getPortSync and getPort.makeRange?

During our migration to webpack v5, we ran into a problem with deasync, as it stopped working. Previously, it was used to convert our webpack.config.js from asynchronous to synchronous. However, since webpack supports asynchronous configurations, I've converted our config file accordingly. However, this change introduced a bottleneck.

As part of our adherence to best practices, the legacy !!raw-loader! syntax has been replaced with ?raw, such as:

const basicSource = require('!!raw-loader!./basic_window_event');
// is now
const basicSource = require('./basic_window_event?raw');

You can read more about this transition in the Webpack documentation.

However, we ran into a problem: eslint does not recognize resourceQuery. This is because eslint-import-resolver-webpack only supports synchronous webpack configurations (see here), while we are using an asynchronous one. Since deasync is no longer working in webpack.config.js, we can't go back to it.

To address this, we could potentially switch back to a synchronous configuration in our webpack.config.js. This could be accomplished by eliminating the use of getPortSync and using port 8030 directly. Would it be possible to simply use port 8030 instead of getPortSync and getPort.makeRange?

2. TypeScript refuses to export the Query class from index.ts for reasons that remain unclear. See the specific issue in the [code here].

The system throws the following warning message:

WARNING in ../../src/components/search_bar/search_bar.tsx 256:39-44
export 'Query' (imported as 'Query') was not found in './query' (possible exports: AST)
 @ ../../src/components/search_bar/index.ts 31:0-49 31:0-49 31:0-49
 @ ../../src/components/index.js 94:0-49 94:0-49 94:0-49
 @ ./routes.js 56:0-56 205:44-60
 @ ./index.js 42:0-30 58:28-47

For now, the issue has been resolved by importing the Query class from query/query.ts, instead of from query/index.ts.

There is a chance that a future update of the `TypeScript' version may have a fix for this problem.

3. Running into a complex type conflict with searchProps in OuiSelectable

We've encountered a type conflict problem with searchProps in the OuiSelectable component that is proving difficult to resolve. Details about the specific type error can be found here.

This problem is currently solved by using the @ts-ignore. There is a chance that a future update of the `TypeScript' version may have a fix for this problem.

4. Build process displays WARNING (@babel/preset-env): The corejs option only has an effect when the useBuiltIns option is not false.

Please note that the current warning does not cause a functional failure. For more details, please refer to the discussion in this thread.

5. Including usedExports in webpack.config.js. For more details, please refer to the discussion in this thread

Thanks guys; this is a big deal and what you have done is awesome. I will pull this down and play with it a bit.

Thanks guys; this is a big deal and what you have done is awesome. I will pull this down and play with it a bit.

Additionally, make sure to test with Dashboards. I was running into issues where one of the auxiliary bundles wasn't exporting anything, causing issues in Dashboards. More info here: https://github.com/opensearch-project/oui/pull/765#issuecomment-1557553908

@andreymyssak I took a shot at resolving the conflicts, but take a look at https://github.com/opensearch-project/oui/pull/770/commits/22fb706223e0a6d73a363b931dcb224ad3412b15 to see if they make sense.

Thinking about this more, could it be possible that the sass-vars-to-js-loader loader is using some sort of Webpack 4 structure and the reason why the charts module doesn't export anything?

A few observations, after rebasing and resolving conflicts:

1. The updated autoprefixer causes duplicate CSS. I am not worried about this because i will be porting some changes from OSD that will eliminate the need for autoprefixer.
2. *.min.css files are no longer minified. postcss and cssnano have both been bumped but I didn't dig to see which portion is breaking the logic.
3. oui.js is 1.5MB larger; doesn't sound right and more investigation is needed.
4. oui.min.js, oui.min.js.LICENSE.txt, oui.min.js.map fail to generate.
5. *ui_charts_theme.js are generated as folders and not files.

I have pushed the rebased code to https://github.com/AMoo-Miki/oui/commit/ddf16ef7cb8ccd24810d618929965432425e7b3e.

Fixed the minification:

• Had to disable cssDeclarationSorter because it could have been unsafe.
• The new cssnano merges back-to-back media queries and removes the media queries for the lower boundary; verified that it was safe.
• The new cssnano also doesn't break and combine neighboring definitions.
• rgba is converted to hsla; shouldn't be a big deal.
• svgo produces slightly longer assets.

• svgo produces slightly longer assets.

But the assets generated by svgo should all be in separate files right? All OUI icons are loaded in async, and won't be in the main bundle, right?

But the assets generated by svgo should all be in separate files right? All OUI icons are loaded in async, and won't be in the main bundle, right?

OUI inlines them :/

postcss.config.js has postcss-inline-svg defined in it. However, the ones I saw elongated are those that it finds in imported deps.

In an attempt to dig deeper into why the JS files were larger with webpack5, i split the eui_charts_theme.js files, before (260KB) and after (363KB). I see a lot more imports from corjs included with webpack5; 57 files totaling 100KB. These include stuff like:

../node_modules/core-js/internals/array-from.js
../node_modules/core-js/internals/iterator-define.js
../node_modules/core-js/internals/regexp-exec.js
../node_modules/core-js/modules/es.symbol.constructor.js

Also, one almost 5KB is wasted due to two indented copyright headers!

I suspect the same is happening with oui.js too.

My latest stuff can be found at https://github.com/AMoo-Miki/oui/tree/updated-webpack-5.

With oui.js:

• 1.5MB was added.
• 197 imports of 461KB are removed.
  • some removed imports are index.ts and types.ts files that only re-exported stuff:

    ./components/delay_hide/index.ts
    ./services/color_picker/index.ts
    ./services/popover/index.ts
    

• 62 imports of 214KB are added.
  • imports from assert make up 114KB of that.
  • 15 are from corejs worth 20KB.
• 1919 files have some kind of change in them, resulting in the 1.7MB delta:
  • Numerous files (i stopped counting at 50 of the first 100) have gained 5KB-10KB, all because of lines like:

  /* harmony import */ var core_js_modules_es_array_slice_js__WEBPACK_IMPORTED_MODULE_21__ = __webpack_require__(/*! core-js/modules/es.array.slice.js */ "../node_modules/core-js/modules/es.array.slice.js");
  /* harmony import */ var core_js_modules_es_array_slice_js__WEBPACK_IMPORTED_MODULE_21___default = /*#__PURE__*/__webpack_require__.n(core_js_modules_es_array_slice_js__WEBPACK_IMPORTED_MODULE_21__);
  

by the way, i couldn't get the docs to work. yarn start shows a blank page.

by the way, i couldn't get the docs to work. yarn start shows a blank page.

From CI, it looks like there's still reference to raw-loader: https://github.com/opensearch-project/oui/actions/runs/6177159690/job/16767685139?pr=770#step:5:31