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

Replace jquery usage in console plugin with native methods #3679

Closed
1 of 4 tasks
Tracked by #3658
joshuarrrr opened this issue Mar 23, 2023 · 5 comments · Fixed by #3733
Closed
1 of 4 tasks
Tracked by #3658

Replace jquery usage in console plugin with native methods #3679

joshuarrrr opened this issue Mar 23, 2023 · 5 comments · Fixed by #3733
Assignees
Labels
console good first issue Good for newcomers help wanted Community development is encouraged refactor Tech debt related tasks that need refactoring technical debt If not paid, jeapardizes long-term success and maintainability of the repository.

Comments

@joshuarrrr
Copy link
Member

joshuarrrr commented Mar 23, 2023

  • Understand why and how jquery is used in the plugin (add as comments to this issue)
  • Replace $() select functions with native methods and properties
  • Replace $.* utility methods (for example, $.each(), $.map(), $.Deferred(), $.when(), $.inArray(), $.ajax())
  • Remove imports

Prioritize runtime code first, then tests

Imported in:

src/plugins/console/public/application/models/legacy_core_editor/legacy_core_editor.ts:
  30  
  31  import ace from 'brace';
  32  import { Editor as IAceEditor, IEditSession as IAceEditSession } from 'brace';
  33: import $ from 'jquery';
  34  import {
  35    CoreEditor,
  36    Position,

src/plugins/console/public/application/models/legacy_core_editor/__tests__/input.test.js:
  31  import '../legacy_core_editor.test.mocks';
  32  import RowParser from '../../../../lib/row_parser';
  33  import { createTokenIterator } from '../../../factories';
  34: import $ from 'jquery';
  35  import { create } from '../create';
  36  
  37  describe('Input', () => {

src/plugins/console/public/application/models/legacy_core_editor/__tests__/output_tokenization.test.js:
  29   */
  30  
  31  import '../legacy_core_editor.test.mocks';
  32: import $ from 'jquery';
  33  import RowParser from '../../../../lib/row_parser';
  34  import ace from 'brace';
  35  import { createReadOnlyAceEditor } from '../create_readonly';

src/plugins/console/public/application/models/sense_editor/sense_editor.test.mocks.ts:
  32  
  33  import '../legacy_core_editor/legacy_core_editor.test.mocks';
  34  
  35: import jQuery from 'jquery';
  36  jest.spyOn(jQuery, 'ajax').mockImplementation(
  37    () =>
  38      new Promise(() => {

src/plugins/console/public/application/models/sense_editor/__tests__/integration.test.js:
  31  import '../sense_editor.test.mocks';
  32  import { create } from '../create';
  33  import _ from 'lodash';
  34: import $ from 'jquery';
  35  
  36  import * as osd from '../../../../lib/osd/osd';
  37  import * as mappings from '../../../../lib/mappings/mappings';

src/plugins/console/public/application/models/sense_editor/__tests__/sense_editor.test.js:
  30  
  31  import '../sense_editor.test.mocks';
  32  
  33: import $ from 'jquery';
  34  import _ from 'lodash';
  35  
  36  import { create } from '../create';

src/plugins/console/public/lib/ace_token_provider/token_provider.test.ts:
  30  
  31  import '../../application/models/sense_editor/sense_editor.test.mocks';
  32  
  33: import $ from 'jquery';
  34  
  35  // TODO:
  36  // We import from application models as a convenient way to bootstrap loading up of an editor using

src/plugins/console/public/lib/mappings/mappings.js:
  28   * under the License.
  29   */
  30  
  31: import $ from 'jquery';
  32  import _ from 'lodash';
  33  import * as opensearch from '../opensearch/opensearch';
  34  

src/plugins/console/public/lib/osd/osd.js:
  38    UsernameAutocompleteComponent,
  39  } from '../autocomplete/components';
  40  
  41: import $ from 'jquery';
  42  import _ from 'lodash';
  43  
  44  import Api from './api';
@joshuarrrr joshuarrrr added good first issue Good for newcomers help wanted Community development is encouraged refactor Tech debt related tasks that need refactoring technical debt If not paid, jeapardizes long-term success and maintainability of the repository. and removed untriaged labels Mar 23, 2023
@kappassov
Copy link
Contributor

Looking at the mapping.js file, jQuery is mostly used for traversing through array or objects and getting specific data. They could be easily replaced by built-in Array.prototype.map() / Array.prototype.forEach(). However, at the end of the file, there are specific methods like $.Deferred(). I would replace it with a Promise with reject/resolve. Consequently, jQuery's $when().done() could be replace by Promise.all().

I am not sure about the following line though:

$(mappingObj).trigger('update', [mappingsResponse, aliases[0]]);

dispatchEvent() might be used for that.

@Nicksqain
Copy link
Contributor

Can I take up this Issue?

@Nicksqain
Copy link
Contributor

The plugin uses the $() function, which is needed to create a jquery object to use methods like .show() and hide() and others ( see below )

.show() ==> el.style.display = ''
.hide() ==> el.style.display = 'none'
.height() ==> el.getBoundingClientRect().height
.offset().top ==> el.offsetTop
.css() ==> el.style
$.map() ==> .map()
$.each() ==> .forEach() (p.s. remember to swap the arguments because forEach has the index as the second argument)
$.Deffered() ==> new Promise
$.when().done() ==> Promise.all().then()
$.ajax() ==> .fetch().then().catch()
$().trigger() ==> new CustomEvent && el.dispatchEvent(new CustomEvent)

@joshuarrrr
Copy link
Member Author

joshuarrrr commented Mar 30, 2023

@Nicksqain that approach looks right to me - we'll take a look at the PR shortly.

@joshuarrrr
Copy link
Member Author

@Nicksqain There may be some overlap with #3775 - we'll likely merge that first and then need to update #3733 accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console good first issue Good for newcomers help wanted Community development is encouraged refactor Tech debt related tasks that need refactoring technical debt If not paid, jeapardizes long-term success and maintainability of the repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants