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

added elastic search plugin #691

Closed
wants to merge 2 commits into from
Closed

added elastic search plugin #691

wants to merge 2 commits into from

Conversation

Sukriti-sood
Copy link

Fixes #478

@Sukriti-sood Sukriti-sood requested a review from a team as a code owner October 30, 2021 09:27
@Sukriti-sood Sukriti-sood changed the base branch from master to develop October 30, 2021 09:28
@Sukriti-sood
Copy link
Author

@olivermrbl Please review the PR

packages/medusa-plugin-elasticsearch/src/loaders/index.js Outdated Show resolved Hide resolved
packages/medusa-plugin-elasticsearch/Readme.md Outdated Show resolved Hide resolved
packages/medusa-plugin-elasticsearch/Readme.md Outdated Show resolved Hide resolved
@Sukriti-sood Sukriti-sood requested a review from srindom November 11, 2021 05:20
@olivermrbl olivermrbl self-requested a review November 11, 2021 09:23
Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

Did you manage to run it locally and add documents? I am experiencing some issues with docs. not being added on load :)

Additionally, would be great if we could add linting. Run yarn add eslint@latest and format it :)


this.options_ = options

this.client_ = new Client(options.config)
Copy link
Contributor

Choose a reason for hiding this comment

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

ElasticSearch Client mutates the config when passed in raw (see here). This means, that ElasticSearch will complain the second time we try to init a client, since its two different objects.

Instead we should add a function:

getConfig() {
    return { ...this.options_.config }
}

And use this when initializing the function: this.client_ = new Client(options.config)

Hope this makes sense :)

})
}

updateSettings(indexName, settings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ElasticSearch will complain if the index does not exist. Let's add a try / catch and create an index, if we receive an error 404 and a message saying that the index is not present. Something along the lines of:

async updateSettings(indexName, settings) {
    try {
      return await this.client_.indices.putSettings({
        index: indexName,
        body: { settings: settings },
      })
    } catch (error) {
      if (error.status === 404) {
        await this.createIndex(indexName, {})
        return await this.updateSettings(indexName, settings)
      }
    }
  }

"description": "Elasticsearch Plugin for Medusa to search for Products",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a couple of build scripts:

"build": "babel src --out-dir dist/ --ignore **/__tests__",
"prepare": "cross-env NODE_ENV=production npm run build",
"watch": "babel -w src --out-dir dist/ --ignore **/__tests__",

@Sukriti-sood
Copy link
Author

Did you manage to run it locally and add documents? I am experiencing some issues with docs. not being added on load :)

Additionally, would be great if we could add linting. Run yarn add eslint@latest and format it :)

Currently, I didn't test them, I will test them on weekend.

@Keith-Hon
Copy link

Hi @Sukriti-sood @olivermrbl any updates on this?

@riqwan
Copy link
Contributor

riqwan commented Jul 5, 2024

Hey, thanks for the PR! Since v2 brought a lot of architectural and API changes on the backend, we will be closing this ticket.

@riqwan riqwan closed this Jul 5, 2024
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.

medusa-plugin-elasticsearch
5 participants