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.

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

Fix exports is undefined #826

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

Hailong-am
Copy link
Collaborator

@Hailong-am Hailong-am commented Jul 24, 2023

Description

Remove babel override for build and keep it for unit test, based on that add a condition in babel.config.js file.

Issues Resolved

related to #741
image

Check List

  • 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.

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #826 (73c7b4e) into main (cdb2f21) will decrease coverage by 0.01%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
- Coverage   63.37%   63.36%   -0.01%     
==========================================
  Files         341      341              
  Lines       11545    11553       +8     
  Branches     2108     2112       +4     
==========================================
+ Hits         7317     7321       +4     
- Misses       3654     3658       +4     
  Partials      574      574              

see 1 file with indirect coverage changes

@AMoo-Miki
Copy link
Contributor

AMoo-Miki commented Jul 27, 2023

I don't believe this is the best solution. While it does solve the problem caused by the typeof transformer, the root cause is the inclusion of @osd/std in the bundles.

Doing

import {
  AppMountParameters,
  CoreSetup,
  CoreStart,
-  DEFAULT_APP_CATEGORIES,
  Plugin,
  PluginInitializerContext,
} from "../../../src/core/public";
+import { DEFAULT_APP_CATEGORIES } from "../../../src/core/utils/default_app_categories";

in https://github.com/opensearch-project/index-management-dashboards-plugin/blob/main/public/plugin.ts, avoids bundling all the unnecessary code from @osd/std and ...; I think due to the smaller payload, this is a better solution.

@Hailong-am
Copy link
Collaborator Author

Hailong-am commented Aug 3, 2023

Based on the suggestion of @AMoo-Miki #834 (comment), will remove the babel override of this project babel.config.js for build and use default config from opensearch-dashboard

we still need this file for yarn test:jest, so a condition is added here for unit test.

@Hailong-am Hailong-am added the v2.10.0 Issues targeting release v2.10.0 label Aug 3, 2023
@AMoo-Miki
Copy link
Contributor

I made a bunch of changes to OSD as well to break the factors that contributed to this problem.

…-commonjs

Signed-off-by: Hailong Cui <ihailong@amazon.com>
Signed-off-by: Hailong Cui <ihailong@amazon.com>
@Hailong-am Hailong-am merged commit be25b8b into opensearch-project:main Aug 10, 2023
8 of 10 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 10, 2023
* Add allowTopLevelThis=true option for @babel/plugin-transform-modules-commonjs

Signed-off-by: Hailong Cui <ihailong@amazon.com>

* remove override babel.config.js for build

Signed-off-by: Hailong Cui <ihailong@amazon.com>

---------

Signed-off-by: Hailong Cui <ihailong@amazon.com>
(cherry picked from commit be25b8b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Hailong-am pushed a commit that referenced this pull request Aug 11, 2023
* Add allowTopLevelThis=true option for @babel/plugin-transform-modules-commonjs



* remove override babel.config.js for build



---------


(cherry picked from commit be25b8b)

Signed-off-by: Hailong Cui <ihailong@amazon.com>
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>
opensearch-trigger-bot bot added a commit that referenced this pull request Aug 25, 2023
* Add allowTopLevelThis=true option for @babel/plugin-transform-modules-commonjs

* remove override babel.config.js for build

---------

(cherry picked from commit be25b8b)

Signed-off-by: Hailong Cui <ihailong@amazon.com>
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>
(cherry picked from commit 639df0d)
bowenlan-amzn added a commit that referenced this pull request Sep 22, 2023
* Add allowTopLevelThis=true option for @babel/plugin-transform-modules-commonjs

* remove override babel.config.js for build

---------

(cherry picked from commit be25b8b)

Signed-off-by: Hailong Cui <ihailong@amazon.com>
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>
(cherry picked from commit 639df0d)

Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: bowenlan-amzn <bowenlan23@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x v2.10.0 Issues targeting release v2.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants