Update to Webpack 5

[jmetev1]

Description

I continued from @AbhishekReddy1127 pr to upgrade webpack. It has some import problems. I fixed them.

Issues Resolved

Fixes #584

Check List

• New functionality includes testing.
• New functionality has been documented.
• All tests pass
  • yarn lint
  • yarn test-unit
• 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.

[BSFishy]

This is awesome! Thanks for finishing this up. It was killing me trying to figure out why those imports were breaking.

On a side note, we require DCO to pass, meaning you need to sign your commits (Reference: https://github.com/opensearch-project/oui/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). You might need to rewrite your git history to achieve this. Make sure all commits are signed off so that we can merge (Specifically, acc7c54 and 566e3f4 need to be signed off) :)

[BSFishy]

Additionally, it looks like you also need to update the yarn.lock again. Running yarn install should fix that

As well there are a few tests in scripts/babel/proptypes-from-ts-props that are failing. Updating the snapshots should be fine to fix that.

[jmetev1]

yeah the problem with yarn lock is i can't yarn install due to puppeteer. My issue is discussed here: puppeteer/puppeteer#6622 . They have a solution but i was hoping someone had a simpler way. It looks like puppeteer is only used for accessibility tests which don't appear to be run in ci so maybe i could just take it out? @BSFishy

[BSFishy]

If you'd like, I can do the final cleanup for you, once you sign the commits. I'm going through all of the commands and everything to do one last sanity test. So far, it looks like we have:

1. Update yarn.lock
2. Update snapshots
3. Revert Remove charts theme code #554 to fix build
4. ?? Other things for yarn build-docs or yarn start?

But I don't think we want to take puppeteer out. There is a PR open to enable it in CI, we just haven't gotten around to merging it #641

[BSFishy]

Alright, I've gotten all of the commands working properly. For some reason Github isn't allowing me to push to your branch, so I pushed my changes to here: BSFishy@a033f12

If you include those changes, I'll run an integ sanity test with Dashboards and we can push this through!

[jmetev1]

ok i've pulled in your cleanup. so now you'll run ci? anything else i need to do?

[jmetev1]

also do you have any plans for addressing puppeteer problem? i assume it will just get worse as more people move to apple silicon. i think one option is jumping to playwright

[BSFishy]

ok i've pulled in your cleanup. so now you'll run ci? anything else i need to do?

At this point in time, no. Thanks for your contribution!

Right now, I'm running an integration test with Dashboards to make sure everything works properly. So far, I've found one issue in the way the charts module is generated. I got a fix working and about to retest. I'll ping you when I'm finished with the testing. After that, we should be ready for review.

also do you have any plans for addressing puppeteer problem? i assume it will just get worse as more people move to apple silicon. i think one option is jumping to playwright

At this point in time, not directly. We're on a team-wide modernization campaign, which will include a rewrite of OUI, which most likely will include Playwright instead of Puppeteer (or another similar tool). But as of now, since OUI v2 is on the horizon, the effort to switch to a different browser testing framework is just too much of a burden. (that is, unless a community member does it, but it's not a specific item on the roadmap).

[BSFishy]

Alright, I'm starting to loose my brain and am hoping to pass my current status off to you. It looks like these changes will be mostly compatible with Dashboards, except that the charts theme module doesn't export anything for some reason.

First off, these lines:

oui/scripts/compile-oui.js

Line 314 in a033f12

'webpack --entry-reset ./themes/charts/themes.ts -o dist/oui_charts_theme.js --output-enabled-library-types="commonjs" --config=src/webpack.config.js',

oui/scripts/compile-oui.js

Line 337 in a033f12

'webpack --entry-reset ./themes/charts/themes.ts -o dist/eui_charts_theme.js --output-enabled-library-types="commonjs" --config=src/webpack.config.js',

Should be changed to look like these:

    'webpack --entry-reset ./themes/charts/themes.ts --output-filename oui_charts_theme.js --output-enabled-library-types="commonjs" --config=src/webpack.config.js',

    'webpack --entry-reset ./themes/charts/themes.ts --output-filename eui_charts_theme.js --output-enabled-library-types="commonjs" --config=src/webpack.config.js',

From there, the files are being generated in the correct path, but they aren't exporting anything for some reason (paths are ./dist/oui_charts_theme.js and ./dist/eui_charts_theme.js). I've tried playing around with some Webpack tree shaking settings to no avail. Not sure what the issue is.

[jmetev1]

ok sorry for tagging people. i thought puppeteer failing to install on yarn install meant the whole thing failed. but turns out everything else works fine even with that failing.

[jmetev1]

that compile script is intimidating. maybe you have someone else who knows more about it?

[jmetev1]

i don't think i have enough context to move forward. What was charts theme? Why was it taken out and now put back in? When was the last time it was built correctly? How can i tell if it gets output correctly? Is there a test that checks that?

[andreymyssak]

ok sorry for tagging people. i thought puppeteer failing to install on yarn install meant the whole thing failed. but turns out everything else works fine even with that failing.

I recently ran into problems with a11y-tests, my workstation on an Apple M2, and I was getting the following error No local server found. Expecting localhost:9999 or localhost:8080 to resolve. I used the solution puppeteer/puppeteer#6622 (comment), but with the following modifications:

browser = await puppeteer.launch({
  headless: false,
  args: ['--no-sandbox'],
  executablePath: '/opt/homebrew/bin/chromium',
});

/*
await page.close();
await browser.close();
*/
const pages = await browser.pages();
for (let i = 0; i < pages.length; i++) {
  await pages[i].close();
}
await browser.close();

After that, the tests were successfully run.

[jmetev1]

good to know!

[BSFishy]

What was charts theme? Why was it taken out and now put back in?

Charts theme is some JS code that creates the theme used in charts. Back before the fork, when this was still EUI, there was a much closer tie between EUI and Elastic Charts. Now, however, we use vega-lite in Dashboards. Charts theme is legacy code that is still used in a few places, so we can't outright delete it yet.

A little while ago, I noticed these files were still in the project so I thought we could get rid of them in the next major version. However, we have pivoted our approach and our next major version will be a complete rewrite of OUI, so it doesn't make sense to be planning the next major version in this repository.

I'm in the process of writing an RFC for OUI v2, which will eventually go out in issues. I've got some other work on my plate before I can finish it, though, so just keep your eye on issues ;)

When was the last time it was built correctly?

OUI builds correctly on all branches, except this one. Since we have such a significant architectural change, we can expect builds to be failing for a bit before we iron out the kinks.

How can i tell if it gets output correctly? Is there a test that checks that?

Usually, I spin up Node, try to import it, and see if it actually exports anything. That usually outputs something along these lines:

~/oui$ node
Welcome to Node.js v14.20.0.
Type ".help" for more information.
> require('./dist/oui_charts_theme')
{}

I'm sure you could write a script that could check for you using a few less keystrokes. It might look something like this:

const pkg = require('./dist/oui_charts_theme');
const keys = Object.keys(pkg);

if (keys.length > 0) {
  console.log('Charts theme exports things correctly');
} else {
  console.error('Charts theme does not export anything');
}

[jmetev1]

Thanks. but i should close this since there's already newer one right? #770

[BSFishy]

Yeah this can probably be closed. If you're still interested in helping with this, please feel free to help in that PR!