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

Jump to bottom

Change deps to keep in preinstall script #852

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Change deps to keep in preinstall script #852

merged 1 commit into from
Jun 28, 2023

Conversation

BSFishy
Copy link
Contributor

@BSFishy BSFishy commented Jun 27, 2023

Description

Adds deps to keep for deps that are needed in a local OUI Dashboards setup. Also removes @types/react, as it causes type errors in Dashboards with differently typed versions of React.

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
Fix local Dashboards link
3dd7d0e 
Signed-off-by: Matt Provost <[email protected]>
SergeyMyssak
SergeyMyssak approved these changes Jun 28, 2023
View reviewed changes
Copy link
Collaborator

@SergeyMyssak SergeyMyssak left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not sure if we should remove react from typesToKeep, what type errors are consoled, and at what stage? I can't fully build it because of the eui_chart_theme file. I'm getting the next error:

ERROR in ./entry.js
Module not found: Error: Can't resolve '@elastic/eui/dist/eui_charts_theme' in '/Users/sergeymyssak/Desktop/OpenSearch-Dashboards/packages/osd-ui-shared-deps'

But it's not a blocker anyway to merge this PR.

@AMoo-Miki AMoo-Miki merged commit 52abc57 into opensearch-project:main Jun 28, 2023
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jun 28, 2023
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 28, 2023
@github-actions
Fix local Dashboards link (#852)
4fa5208 
Signed-off-by: Matt Provost <[email protected]>
(cherry picked from commit 52abc57)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@AMoo-Miki
Copy link
Collaborator

LGTM, but I'm not sure if we should remove react from typesToKeep, what type errors are consoled, and at what stage? I can't fully build it because of the eui_chart_theme file. I'm getting the next error:

ERROR in ./entry.js
Module not found: Error: Can't resolve '@elastic/eui/dist/eui_charts_theme' in '/Users/sergeymyssak/Desktop/OpenSearch-Dashboards/packages/osd-ui-shared-deps'

But it's not a blocker anyway to merge this PR.

The error you are getting is odd; @BSFishy could you please take a look at this with Sergey?

@BSFishy
Copy link
Contributor Author

BSFishy commented Jun 28, 2023

LGTM, but I'm not sure if we should remove react from typesToKeep, what type errors are consoled, and at what stage? I can't fully build it because of the eui_chart_theme file. I'm getting the next error:

Before this change, if you tried to run yarn osd bootstrap with all OUI versions set to file:<absolute path to OUI>, you would get these errors:

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/pretty_duration.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/super_date_picker.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/relative_utils.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/date_modes.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/date_popover/absolute_tab.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/date_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/date_popover/relative_tab.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/date_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/quick_select_popover/quick_select.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/quick_select_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/quick_select_popover/quick_select_utils.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/components/date_picker/super_date_picker/quick_select_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/pretty_duration.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/super_date_picker.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/relative_utils.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/date_modes.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/date_popover/relative_tab.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/date_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/date_popover/absolute_tab.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/date_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/quick_select_popover/quick_select.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/quick_select_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/quick_select_popover/quick_select_utils.js
            Module not found: Error: Can't resolve '@opensearch/datemath' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/es/eui_components/date_picker/super_date_picker/quick_select_popover'

            ERROR in [super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/node_modules/use-sidecar/dist/es2015/env.js
            Module not found: Error: Can't resolve 'detect-node' in '[super secret path]/OpenSearch-Dashboards/node_modules/@elastic/eui/node_modules/use-sidecar/dist/es2015'

Adding @opensearch and detect-node fixed those errors and allowed the bootstrap to get further, but then we'd run into a case where React types were conflicting. Essentially, OUI and OSD use different versions of @types/react, so OSD was expecting new versions of those types where OUI specified the old versions. I don't have the errors on hand, but they looked something like this:

TypeError: Expected type `ReactType`, found `ReactType`

So the error message isn't actually any useful in figuring out the root cause, but I've luckily run into a similar case previously to be able to diagnose this. I admit that ideally we're providing accurate types with OUI, however given that it's a React component library, we can reasonably assume that clients will be using React, and if they need React types, they'll specify the version they need on their end. For example, Typescript should use the types that OSD provides since OUI doesn't provide any.

The error you are getting is odd; @BSFishy could you please take a look at this with Sergey?

Yeah, this is merging into main, so I needed to manually revert the commits that removed charts. It was only 2 commands, so not very difficult, but I think we might want to look more into pushing #831 so it's a little easier working off of main

@BSFishy BSFishy deleted the fix_preinstall branch June 28, 2023 19:03
BSFishy pushed a commit that referenced this pull request Jun 29, 2023
@opensearch-trigger-bot @github-actions
Fix local Dashboards link (#852) (#861)
28c59b7 
(cherry picked from commit 52abc57)

Signed-off-by: Matt Provost <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyMyssak SergeyMyssak SergeyMyssak approved these changes

@AMoo-Miki AMoo-Miki AMoo-Miki approved these changes

@seanneumann seanneumann Awaiting requested review from seanneumann

@rednaksi91 rednaksi91 Awaiting requested review from rednaksi91

@KrooshalUX KrooshalUX Awaiting requested review from KrooshalUX

@bandinib-amzn bandinib-amzn Awaiting requested review from bandinib-amzn bandinib-amzn is a code owner

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@joshuarrrr joshuarrrr Awaiting requested review from joshuarrrr joshuarrrr is a code owner

@andreymyssak andreymyssak Awaiting requested review from andreymyssak

Assignees
No one assigned
Labels
backport 1.x distinguished-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BSFishy @AMoo-Miki @SergeyMyssak