Skip to content

Update 3.0.0 qualifier from alpha1 to beta1 [DO NOT MERGE] #1291

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

Closed
wants to merge 10 commits into from

Conversation

vaibhoag
Copy link
Contributor

@vaibhoag vaibhoag commented Mar 31, 2025

Description

Update 3.0.0 qualifier from alpha1 to beta1

Issues Resolved

[List any issues this PR will resolve]

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.

Hailong-am
Hailong-am previously approved these changes Mar 31, 2025
"cypress": "12.17.4",
"cypress": "^13.6.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason upgrade cypress version? i see CI was failing because version conflicts

cypress
          12.17.4 => opensearch-dashboards
          ^13.6.0 => opensearch_index_management_dashboards

Copy link
Contributor Author

@vaibhoag vaibhoag Mar 31, 2025

Choose a reason for hiding this comment

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

This is unusual , because when I ran 'yarn osd bootstrap', it showed me the following error:

      The conflicting dependencies are:
        cypress
          ^13.6.0 => opensearch-dashboards
          12.17.4 => opensearch_index_management_dashboards

To fix this, I updated the cypress version.

I referred to line 414 of https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/package.json

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

honestly my recommendation is just delete this line completely and rely on the reference in OSD package.json

@Hailong-am
Copy link
Collaborator

make sure CIs are passed before merge

Signed-off-by: Vaibhav Agarwal <[email protected]>
@vaibhoag
Copy link
Contributor Author

vaibhoag commented Apr 2, 2025

@kavilla @angle943 @d-buckner @peterzhuamazon We see that Run binary Installation test is not able pass because of the changes in Bump monaco-editor from 0.17.0 to 0.30.1.
Can you please look into this and tell what changes need to done to resolve the failure?

Signed-off-by: Vaibhav Agarwal <[email protected]>
@ruchidh
Copy link

ruchidh commented Apr 2, 2025

@kavilla @angle943 @d-buckner @peterzhuamazon We see that Run binary Installation test is not able pass because of the changes in Bump monaco-editor from 0.17.0 to 0.30.1. Can you please look into this and tell what changes need to done to resolve the failure?

This error occurs because the ?? (nullish coalescing operator) is not being handled correctly during build
if you can add @babel/plugin-proposal-nullish-coalescing-operator in bable plugins, it might help

vaibhoag added 2 commits April 3, 2025 12:55
Signed-off-by: Vaibhav Agarwal <[email protected]>
Signed-off-by: Vaibhav Agarwal <[email protected]>
@angle943
Copy link

angle943 commented Apr 3, 2025

the error is still happening. The error happens when running @osd/optimizer. Perhaps the babel polyfill neeeds to be implemented in osd-optimizer?

BTW, we should not be using @babel/plugin-proposal-nullish-coalescing-operator, as it is deprecated: https://www.npmjs.com/package/@babel/plugin-proposal-nullish-coalescing-operator . It recommends we use @babel/plugin-transform-nullish-coalescing-operator

@ananzh
Copy link
Member

ananzh commented Apr 3, 2025

@kavilla @angle943 @d-buckner @peterzhuamazon We see that Run binary Installation test is not able pass because of the changes in Bump monaco-editor from 0.17.0 to 0.30.1. Can you please look into this and tell what changes need to done to resolve the failure?

We have bumped to 0.52.0. Please refer to these documents:
RFC: opensearch-project/OpenSearch-Dashboards#9642
Guidance: opensearch-project/OpenSearch-Dashboards#9654

I have reproduced the error. The original issue that the index-management is directly importing the Monaco editor's JSON mode from monaco-editor/esm/vs/language/json/jsonMode.js, which contains the nullish coalescing operator (??) that's causing the build error.

Why just adding the Babel plugin didn't work

When you added @babel/plugin-proposal-nullish-coalescing-operator to both babel.config.js and opensearch_dashboards.json, it should theoretically have allowed the build process to handle the nullish coalescing operator (??) syntax. However, the build still failed because:

  • The problematic code was in an external package: The error was occurring in a file from the monaco-editor package (monaco-editor/esm/vs/language/json/jsonMode.js) that was being directly imported by your plugin.

  • Webpack loader chain issue: When webpack processes imports, it applies loaders based on file paths and patterns. The Babel configuration you added was likely not being applied to the monaco-editor files because they're in node_modules, which are often excluded from transformation.

  • Direct import bypassing OpenSearch Dashboards' Monaco wrapper: Your plugin was directly importing from monaco-editor instead of using the OpenSearch Dashboards wrapper (@osd/monaco), which would have handled these compatibility issues.

How to make it work

Instead of trying to make the build process handle the problematic syntax, we could use a different approach:

  • Removed the direct import: We completely rewrote the json.ts file to avoid importing from monaco-editor/esm/vs/language/json/jsonMode.js, which contained the nullish coalescing operator.

  • Used OpenSearch Dashboards' Monaco wrapper: We modified the code to use only the Monaco instance provided by OpenSearch Dashboards (@osd/monaco), which is already properly configured and processed.

  • Maintained the same interface: We kept the same class and method signatures to ensure backward compatibility with the rest of your plugin code.

Why this approach works better

  • Avoids the problem entirely: Rather than trying to fix the build configuration to handle the syntax, we removed the problematic code import altogether.
  • Follows best practices: It aligns with how OpenSearch Dashboards expects plugins to use Monaco editor (through the @osd/monaco package).
  • More robust solution: It doesn't rely on complex build configuration changes that might be overridden or ignored in certain contexts.

This is a common pattern in software development - sometimes it's better to avoid a problem entirely rather than trying to fix it, especially when dealing with build configurations and third-party packages.

Detailed changes

  • plugins/index-management-dashboards-plugin/babel.config.js
/*
 * Copyright OpenSearch Contributors
 * SPDX-License-Identifier: Apache-2.0
 */

// babelrc doesn't respect NODE_PATH anymore but using require does.
// Alternative to install them locally in node_modules
module.exports = function (api) {
  const isTest = api.env("test");
  
  // Common plugins for all environments
  const commonPlugins = [
    require("@babel/plugin-proposal-nullish-coalescing-operator"),
  ];
  
  // Test-specific configuration
  if (isTest) {
    return {
      presets: [require("@babel/preset-env"), require("@babel/preset-react"), require("@babel/preset-typescript")],
      plugins: [
        [require("@babel/plugin-transform-runtime"), { regenerator: true }],
        require("@babel/plugin-transform-class-properties"),
        require("@babel/plugin-transform-object-rest-spread"),
        [require("@babel/plugin-transform-modules-commonjs"), { allowTopLevelThis: true }],
        ...commonPlugins,
      ],
    };
  }
  
  // Build/dev configuration
  return {
    plugins: commonPlugins,
  };
};
  • plugins/index-management-dashboards-plugin/public/lib/monaco/json.ts
import { monaco } from "@osd/monaco";

// Use the existing Monaco JSON language features
// This ensures we're using the version that's already configured in OpenSearch Dashboards

// Make sure JSON language is registered
if (!monaco.languages.getLanguages().some(lang => lang.id === 'json')) {
  monaco.languages.register({
    id: "json",
    extensions: [".json", ".bowerrc", ".jshintrc", ".jscsrc", ".eslintrc", ".babelrc"],
    aliases: ["JSON", "json"],
    mimetypes: ["application/json"],
  });
}

// Set default diagnostics options if json language is available
if (monaco.languages.json) {
  const diagnosticDefault = {
    validate: true,
    allowComments: true,
    schemas: [],
    enableSchemaRequest: false,
  };
  
  // Only set diagnostics options if the method exists
  if (monaco.languages.json.jsonDefaults && 
      typeof monaco.languages.json.jsonDefaults.setDiagnosticsOptions === 'function') {
    monaco.languages.json.jsonDefaults.setDiagnosticsOptions(diagnosticDefault);
  }
}

// Export a compatible interface for backward compatibility
export class LanguageServiceDefaultsImpl {
  _onDidChange: monaco.Emitter<any>;
  _languageId: string;
  _diagnosticsOptions!: monaco.languages.json.DiagnosticsOptions;
  
  constructor(languageId: string, diagnosticsOptions: monaco.languages.json.DiagnosticsOptions) {
    this._onDidChange = new monaco.Emitter();
    this._languageId = languageId;
    this.setDiagnosticsOptions(diagnosticsOptions);
  }
  
  public get languageId() {
    return this._languageId;
  }
  
  public get onDidChange() {
    return this._onDidChange.event;
  }
  
  public get diagnosticsOptions() {
    return this._diagnosticsOptions;
  }
  
  setDiagnosticsOptions(options: monaco.languages.json.DiagnosticsOptions) {
    this._diagnosticsOptions = options || Object.create(null);
    this._onDidChange.fire(this);
    
    // If monaco.languages.json exists, also update its diagnostics options
    if (monaco.languages.json && 
        monaco.languages.json.jsonDefaults && 
        typeof monaco.languages.json.jsonDefaults.setDiagnosticsOptions === 'function') {
      monaco.languages.json.jsonDefaults.setDiagnosticsOptions(options);
    }
  }
}

I have run yarn build-platform --linux --skip-os-packages --release in my ubuntu env to verify.

vaibhoag added 2 commits April 4, 2025 00:28
Signed-off-by: Vaibhav Agarwal <[email protected]>
Signed-off-by: Vaibhav Agarwal <[email protected]>
@vaibhoag
Copy link
Contributor Author

vaibhoag commented Apr 3, 2025

@ananzh @peterzhuamazon @ruchidh Looks like JSON mode setup that was previously handled by importing the jsonMode.js , that removes a key feature of createIndex of having a JSON Editor for creating index.

I checked in with UI, Jsoneditor is no longer available.
if we remember the error, this is how we reach to jsonMode.js

|   let indentValue;
      │          |   if (options.insertSpaces) {
      │          >     indentValue = cachedSpaces[options.tabSize || 4] ?? repeat(cachedSpaces[1], options.tabSize || 4);
      │          |   } else {
      │          |     indentValue = "	";
      │           @ ./public/lib/monaco/json.ts 6:0-79 12:17-27
      │           @ ./public/lib/monaco/index.ts
      │           @ ./public/components/MonacoJSONEditor/hooks.tsx
      │           @ ./public/components/MonacoJSONEditor/MonacoJSONEditor.tsx
      │           @ ./public/components/MonacoJSONEditor/index.tsx
      │           @ ./public/components/IndexMapping/IndexMapping.tsx
      │           @ ./public/pages/CreateIndex/containers/IndexForm/index.tsx
      │           @ ./public/pages/CreateIndex/containers/CreateIndex/CreateIndex.tsx
      │           @ ./public/pages/CreateIndex/containers/CreateIndex/index.ts
      │           @ ./public/pages/CreateIndex/index.ts

Without jsonMode.js, the JSON Editor functionality breaks, as evidenced by the error trace:
public/lib/monaco/json.ts → MonacoJSONEditor → IndexMapping → CreateIndex

Sorry for not noticing this earlier.
Can you suggest any other way to resolve this issue?

@ananzh
Copy link
Member

ananzh commented Apr 4, 2025

The index-management-dashboards-plugin was failing to build with a specific error related to the nullish coalescing operator (??) in Monaco editor code:

indentValue = cachedSpaces[options.tabSize || 4] ?? repeat(cachedSpaces[1], options.tabSize || 4);

This error occurred in the build process when the plugin tried to use the JSON editor functionality for creating and editing index mappings. The error trace showed the dependency chain:

./public/lib/monaco/json.ts
./public/lib/monaco/index.ts
./public/components/MonacoJSONEditor/hooks.tsx
./public/components/MonacoJSONEditor/MonacoJSONEditor.tsx
./public/components/MonacoJSONEditor/index.tsx
./public/components/IndexMapping/IndexMapping.tsx
./public/pages/CreateIndex/containers/IndexForm/index.tsx

This chain leads to the CreateIndex component, which is a critical part of the plugin that allows users to create and edit index mappings using a JSON editor.

So we have useed @osd/monaco, which follows OpenSearch Dashboards best practices by using the official Monaco wrapper. You will need to make more changes to adapt this change. We should add/modify

  • A custom JsonDiagnosticsOptions type
  • A jsonDefaults implementation with setDiagnosticsOptions method
  • A compatible LanguageServiceDefaultsImpl class
  • A basic JSON completion provider

This can ensure that the JSON editor in the create_index form works correctly without requiring changes to the build configuration or the MonacoJSONEditor component itself.

The key insight was that instead of trying to make the build process handle the nullish coalescing operator, we implemented our own version of the JSON language support that provides the same functionality without using this modern JavaScript syntax feature.

Can you try to update json.ts to

/*
 * Copyright OpenSearch Contributors
 * SPDX-License-Identifier: Apache-2.0
 */
import { monaco } from "@osd/monaco";

// Ensure JSON language is registered
if (!monaco.languages.getLanguages().some((lang) => lang.id === "json")) {
  monaco.languages.register({
    id: "json",
    extensions: [".json", ".bowerrc", ".jshintrc", ".jscsrc", ".eslintrc", ".babelrc"],
    aliases: ["JSON", "json"],
    mimetypes: ["application/json"],
  });
}

// Initialize JSON language features if not already available
if (!monaco.languages.json) {
  // Create a namespace for JSON language features
  (monaco.languages as any).json = {};
}

// Define the diagnostics options type to be used throughout the file
type JsonDiagnosticsOptions = {
  validate: boolean;
  allowComments: boolean;
  schemas: any[];
  enableSchemaRequest: boolean;
  [key: string]: any;
};

// Create jsonDefaults if it doesn't exist
if (!monaco.languages.json.jsonDefaults) {
  const jsonDefaults = {
    _diagnosticsOptions: {
      validate: true,
      allowComments: true,
      schemas: [],
      enableSchemaRequest: false,
    } as JsonDiagnosticsOptions,
    _onDidChange: new monaco.Emitter<JsonDiagnosticsOptions>(),
    
    get diagnosticsOptions(): JsonDiagnosticsOptions {
      return this._diagnosticsOptions;
    },
    
    setDiagnosticsOptions(options: JsonDiagnosticsOptions) {
      this._diagnosticsOptions = { ...this._diagnosticsOptions, ...options };
      this._onDidChange.fire(this._diagnosticsOptions);
    },
    
    get onDidChange() {
      return this._onDidChange.event;
    },
  };
  
  // Attach jsonDefaults to monaco.languages.json
  (monaco.languages.json as any).jsonDefaults = jsonDefaults;
}


// Export a compatible interface for backward compatibility
export class LanguageServiceDefaultsImpl {
  _onDidChange: monaco.Emitter<any>;
  _languageId: string;
  _diagnosticsOptions: JsonDiagnosticsOptions;

  constructor(languageId: string, diagnosticsOptions: JsonDiagnosticsOptions) {
    this._onDidChange = new monaco.Emitter();
    this._languageId = languageId;
    this._diagnosticsOptions = diagnosticsOptions || Object.create(null);
    this.setDiagnosticsOptions(diagnosticsOptions);
  }

  public get languageId() {
    return this._languageId;
  }

  public get onDidChange() {
    return this._onDidChange.event;
  }

  public get diagnosticsOptions() {
    return this._diagnosticsOptions;
  }

  setDiagnosticsOptions(options: JsonDiagnosticsOptions) {
    this._diagnosticsOptions = options || Object.create(null);
    this._onDidChange.fire(this);

    // Update monaco.languages.json.jsonDefaults with our options
    if (monaco.languages.json && monaco.languages.json.jsonDefaults) {
      monaco.languages.json.jsonDefaults.setDiagnosticsOptions(options);
    }
  }
}

// Register JSON completion provider if needed
if (monaco.languages.json && !(monaco.languages.json as any)._jsonCompletionProviderRegistered) {
  monaco.languages.registerCompletionItemProvider('json', {
    provideCompletionItems: (model, position, context, token) => {
      // Basic JSON completion provider
      const textUntilPosition = model.getValueInRange({
        startLineNumber: position.lineNumber,
        startColumn: 1,
        endLineNumber: position.lineNumber,
        endColumn: position.column,
      });
      
      const match = textUntilPosition.match(/"([^"]*)":\s*$/);
      if (match) {
        // After property name, suggest values
        return {
          suggestions: [
            {
              label: 'true',
              kind: monaco.languages.CompletionItemKind.Keyword,
              insertText: 'true',
              range: {
                startLineNumber: position.lineNumber,
                startColumn: position.column,
                endLineNumber: position.lineNumber,
                endColumn: position.column,
              }
            },
            {
              label: 'false',
              kind: monaco.languages.CompletionItemKind.Keyword,
              insertText: 'false',
              range: {
                startLineNumber: position.lineNumber,
                startColumn: position.column,
                endLineNumber: position.lineNumber,
                endColumn: position.column,
              }
            },
            {
              label: 'null',
              kind: monaco.languages.CompletionItemKind.Keyword,
              insertText: 'null',
              range: {
                startLineNumber: position.lineNumber,
                startColumn: position.column,
                endLineNumber: position.lineNumber,
                endColumn: position.column,
              }
            },
            {
              label: '{}',
              kind: monaco.languages.CompletionItemKind.Snippet,
              insertText: '{}',
              range: {
                startLineNumber: position.lineNumber,
                startColumn: position.column,
                endLineNumber: position.lineNumber,
                endColumn: position.column,
              }
            },
            {
              label: '[]',
              kind: monaco.languages.CompletionItemKind.Snippet,
              insertText: '[]',
              range: {
                startLineNumber: position.lineNumber,
                startColumn: position.column,
                endLineNumber: position.lineNumber,
                endColumn: position.column,
              }
            }
          ]
        };
      }
      
      return { suggestions: [] };
    }
  });
  
  // Mark as registered to avoid duplicate registration
  (monaco.languages.json as any)._jsonCompletionProviderRegistered = true;
}

@ruchidh
Copy link

ruchidh commented Apr 4, 2025

with above changes from anan and combining below changes .

The issue was with [getModeId] , for we can replace it with getLanguageId, which is the correct method to retrieve the language ID of the model.

originalModel.getModeId(); has to be replace with originalModel.getLanguageId() in the hooks useModel method.
It is working now.

Signed-off-by: Vaibhav Agarwal <[email protected]>
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.89%. Comparing base (f728d7c) to head (6c4848c).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1291      +/-   ##
==========================================
- Coverage   60.92%   60.89%   -0.04%     
==========================================
  Files         348      348              
  Lines       12842    12865      +23     
  Branches     2589     2598       +9     
==========================================
+ Hits         7824     7834      +10     
- Misses       4279     4289      +10     
- Partials      739      742       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Vaibhav Agarwal <[email protected]>
@peterzhuamazon
Copy link
Member

Hi @vaibhoag is this ready to merge now?

Thanks.

@vaibhoag
Copy link
Contributor Author

vaibhoag commented Apr 4, 2025

Hi @vaibhoag is this ready to merge now?

Thanks.

@peterzhuamazon I am working on adapting changes which are coming after cypress version upgrade. I will update on this asap.

@vaibhoag vaibhoag changed the title Update 3.0.0 qualifier from alpha1 to beta1 Update 3.0.0 qualifier from alpha1 to beta1 [DO NOT MERGE] Apr 5, 2025
@vaibhoag vaibhoag closed this Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants