Sort by date

[CaroFG]

No description provided.

[fharper]

I cannot review this since I have issue with my MeiliSearch installation right now, so maybe @curquiza can check it?

[curquiza]

Hello @fharper and @CaroFG!

@fharper you can use docker to start MeiliSearch, do you still have issues on your computer when using docker?

$ docker run -it --rm  -p 7700:7700 getmeili/meilisearch:latest ./meilisearch --no-analytics=true

@CaroFG can you fix the git conflict before we do any review? 🙂

I'll try to review it asap then, but not sure I'll have the time to do it before the end of this week, sorry 😢

[CaroFG]

Conflicts solved!

[fharper]

@fharper you can use docker to start MeiliSearch, do you still have issues on your computer when using docker?

No I can't, it's not stable yet on M1. Actually, Docker releases a tech preview version, I'll see if it's working. and I'll let you know if I can.

I'll try to review it asap then, but not sure I'll have the time to do it before the end of this week, sorry 😢

Don't apologize, it's nice enough of you to help while I can't test things myself yet. We appreciate a lot 💙

[fharper]

@curquiza I'm now able to run Meili on my machine using a compiled version, so unless you have time tomorrow to test this PR since I'm not there, I'll do it next year.

[curquiza]

"next year" 🤣 Like it!
I will not have the time to do it this year either.

[curquiza]

Sorry, my requests for changes are not always related to this PR specifically

1. I just realized, when you arrive at step 3 of the README (Donwload the dataset) you have not asked the user to do an npm install before, so you get an error: Error: Cannot find module 'axios'.
   FYI: npm install is present in step 4 but not before.

2. The footer talks about "SDKs and Integration tools" -> is this repo an integration tool? I would say no, but maybe I'm wrong. If it's not, maybe adapt the footer for the dev rel team would be a good idea (or remove it).

3. In the README at step 4, you write "Create an index called artWorks in your MeiliSearch instance." -> this is not true anymore if I'm not wrong 🙂

---

I tested the demo and it works! Well done!
My request for changes in the code will not impact the visual of your demo 🙂

[curquiza]

Suggested change

	await client.createIndex('artWorksAsc', { primaryKey: 'ObjectID' })
	const indexAsc = client.getIndex('artWorksAsc')
	const indexAsc = await client.createIndex('artWorksAsc', { primaryKey: 'ObjectID' })

createIndex already returns an Index instance 🙂

[CaroFG]

Concerning the README, number 1 had already been corrected in another PR, 2. I reformulated the footer sentence. Let me know what you think, please. 3. Updated it. Thank you for your feedback!!!!

[curquiza]

Suggested change

	await client.createIndex('artWorksDesc', { primaryKey: 'ObjectID' })
	const indexDesc = client.getIndex('artWorksDesc')
	const indexDesc = await client.createIndex('artWorksDesc', { primaryKey: 'ObjectID' })

[CaroFG]

Thank you!!! Corrected :)

[curquiza]

Maybe could you use the forEach method to loop on your array instead of a for loop?
https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/Array/forEach

Your solution is not wrong but not the most idiomatic way to do it in JS, I think this is a good opportunity for you to practice the "functional" way of JavaScript by using the built-in methods the languages provides 🙂

More about looping on an array: https://stackoverflow.com/questions/3010840/loop-through-an-array-in-javascript
A article Avoid Writing Another For-Loop in JavaScript : https://medium.com/better-programming/never-write-another-for-loop-in-javascript-9db11afa6445

[CaroFG]

Well, I tried, and the forEach function happens to have different behavior than the 'for loop' when used with async functions. The forEach method doesn't wait for the callback function to be done before calling the next one, which caused, in my case, MeiliSearch communication errors. So I'll stick to the 'for loop' this time.
But thank you so much for your feedback. I really appreciate the time you take to help me progress and teach me the best practices. :)

[fharper]

My pleasure! Oh wait... 🤣

[curquiza]

Same here about the for loop.

[fharper]

@CaroFG I will let you fix the points @curquiza made before reviewing it.

[curquiza]

👍

There is still the npm install issue in the README, but maybe do you plan to fix it in another PR @CaroFG?

[CaroFG]

There is still the npm install issue in the README, but maybe do you plan to fix it in another PR @CaroFG?

Are you sure about that? Cause it was already solved in main and I rebased...

[curquiza]

Are you sure about that? Cause it was already solved in main and I rebased...

Indeed! My bad Caro, I did not notice you rebased! Sorry! That's perfect 😊

[bidoubiwa]

In the README at step 4, you write "Create an index called artWorks in your MeiliSearch instance." -> this is not true anymore if I'm not wrong 🙂

I think those steps are not what the user is supposed to do but what the script npm run prep-meili will do. Even with the lazy index, the script will create that index (unless I understood the readme badly in which case sorry sorry)

[bidoubiwa]

I would suggest replacing all for loops with maps. But this can be done in another PR as it is may be not the main purpose of this PR.

Good job on this PR !!

[bidoubiwa]

A more modern approach to these function parameters would be to use destructuring in function arguments and spread operators to construct your objects.

destructuring

If you don't know destructuring basically it looks like this:

const obj = { name: "Caro", age: "way older than charlotte" }
const { name, age } = obj;
console.log(name) // Caro
console.log(age) // way older than charlotte

This can also be used in functions the following way:

const user = {
  id: 42,
  displayName: 'jdoe',
  fullName: {
    firstName: 'John',
    lastName: 'Doe'
  }
};

function whois({displayName, fullName: {firstName: name}}) {
  return `${displayName} is ${name}`;
}

console.log(whois(user));  // "jdoe is John"

Spread operators

Spread operators can be used for a variety of things, in our case, we will use it to ease the merge objects togethers.

A quick example with arrays first where instead of using the concat function of javascript, we use the spread operator to merge both arrays.

let arr1 = [0, 1, 2];
let arr2 = [3, 4, 5];

arr1 = [...arr1, ...arr2];
//  arr1 is now [0, 1, 2, 3, 4, 5]

let obj1 = { foo: 'bar', x: 42 };
let obj2 = { foo: 'baz', y: 13 };

let clonedObj = { ...obj1 };
// Object { foo: "bar", x: 42 }

let mergedObj = { ...obj1, ...obj2 };
// Object { foo: "baz", x: 42, y: 13 }

As you can observe the order of the objects is really important, as in this situation let mergedObj = { ...obj1, ...obj2 };, all attributes found both in obj1 and obj2 will be overwritten by => obj2 foo becomes baz in mergedObj.

To adapt that to your code it would look like this:

Suggested change

	async function populateIndex (array, batchedDataSet) {
	settings.rankingRules = array.rules
	const index = array.index
	await index.updateSettings(settings)
	console.log(`Settings added to ${array.name} index.`)
	console.log(`Adding documents to ${array.name}...`)
	for (let i = 0; i < batchedDataSet.length; i++) {
	const { updateId } = await index.addDocuments(batchedDataSet[i])
	await index.waitForPendingUpdate(updateId, {
	timeOutMs: 100000
	})
	}
	}
	async function populateIndex ({ index, rules, name }, batchedDataSet) {
	await index.updateSettings({ ...settings, rankingRules: rules})
	console.log(`Settings added to ${name} index.`)
	console.log(`Adding documents to ${name}...`)
	for (let i = 0; i < batchedDataSet.length; i++) {
	const { updateId } = await index.addDocuments(batchedDataSet[i])
	await index.waitForPendingUpdate(updateId, {
	timeOutMs: 100000
	})
	}
	}

[CaroFG]

Thank you soooo much for all the explanations, I truly appreciate the time you've taken to review my code and help me progress. I'll do a PR for the "for loops"

[bidoubiwa]

Suggested change

	async function indexIsPopulated (index, dataset) {
	const indexStats = await index.getStats()
	if (indexStats.numberOfDocuments === dataset.length) {
	return true
	} else {
	return false
	}
	}
	async function indexIsPopulated (index, dataset) {
	const indexStats = await index.getStats()
	return indexStats.numberOfDocuments === dataset.length
	}

[bidoubiwa]

As mentioned, changing all for into map could be in another PR as it should recieve a particular attention.
Now, we can still talk about the choice of for loop!

Do you know about for..of and for..in

for of

const array1 = ['a', 'b', 'c'];

for (const element of array1) {
  console.log(element);
}

// expected output: "a"
// expected output: "b"
// expected output: "c"

for in

const object = { a: 1, b: 2, c: 3 };

for (const property in object) {
  console.log(`${property}: ${object[property]}`);
}

// expected output:
// "a: 1"
// "b: 2"
// "c: 3"

You can update your code accordingly with something like this:

Suggested change

	for (let i = 0; i < indexArray.length; i++) {
	const isPopulated = await indexIsPopulated(indexArray[i].index, dataset)
	if (isPopulated) {
	console.log(`Index "${indexArray[i].name}" already exists`)
	} else {
	await populateIndex(indexArray[i], batchedDataSet)
	console.log(`Documents added to "${indexArray[i].name}"`)
	}
	for (const index of indexArray) {
	const isPopulated = await indexIsPopulated(index.index, dataset)
	if (isPopulated) {
	console.log(`Index "${index.name}" already exists`)
	} else {
	await populateIndex(index, batchedDataSet)
	console.log(`Documents added to "${index.name}"`)
	}

[bidoubiwa]

Suggested change

	const indexArray = [artWorks, artWorksAsc, artWorksDesc]
	// create Indexes array
	const indexArray = [
	{ name: 'artWorks', index: artWorksIndex, rules: defaultRankingRules },
	{ name: 'artWorksAsc', index: artWorksAscIndex, rules: rankingRulesAsc },
	{ name: 'artWorksDesc', index: artWorksDescIndex, rules: rankingRulesDesc }
	]

[bidoubiwa]

Suggested change

	const artWorks = { name: 'artWorks', index: artWorksIndex, rules: defaultRankingRules }
	const artWorksAsc = { name: 'artWorksAsc', index: artWorksAscIndex, rules: rankingRulesAsc }
	const artWorksDesc = { name: 'artWorksDesc', index: artWorksDescIndex, rules: rankingRulesDesc }

see next suggestion.

[bidoubiwa]

Not accurate comment

Suggested change

// Check if index are populated and populate them is needed

[bidoubiwa]

After this change, I approve this PR :)

[bidoubiwa]

LGTM :) Since there are no tests, could you check if everything still works correctly?

[CaroFG]

LGTM :) Since there are no tests, could you check if everything still works correctly?

Yes, it does