Skip to content

Conversation

bidoubiwa
Copy link
Contributor

@bidoubiwa bidoubiwa commented Oct 24, 2022

fixes: #134

The fieldsToIndex option was missing in the backfill script.

Example now:

    yarn run-cli \
      --project <project_id> \
      --source-collection-path <collection_name> \
      --index <index_uid> \
      --batch-size <100/default=300> \
      --host <host_address> \
      --api-key <api_key> \
      --fields-to-index title, description
      --non-interactive

@bidoubiwa bidoubiwa changed the title Add fieldsToIndex flag on the bin Add fieldsToIndex as option in backfill script Oct 25, 2022
@bidoubiwa bidoubiwa marked this pull request as ready for review October 25, 2022 13:52
@bidoubiwa bidoubiwa requested a review from alallema October 25, 2022 13:53
@bidoubiwa bidoubiwa added the bug Something isn't working label Oct 25, 2022
Copy link
Contributor

@alallema alallema left a comment

Choose a reason for hiding this comment

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

Well done!!! 🎉
I made a few comments

@@ -94,14 +98,15 @@ async function retrieveCollectionFromFirestore(
* Adapts documents and indexes them in Meilisearch.
* @param {any} docs
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we use any instead of DocumentSnapshot[] here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the previous one was a mistake @param {Change<DocumentSnapshot>} change since change is not docs. But you'r right, putting it back :)

const document = adaptDocument(
documentId,
after,
config.meilisearch.fieldsToIndex || ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Same about:

    fieldsToIndex: process.env.MEILISEARCH_FIELDS_TO_INDEX || '',

Is it not the same parameter?

Copy link
Contributor Author

@bidoubiwa bidoubiwa Oct 26, 2022

Choose a reason for hiding this comment

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

because the type of MeilisearchConfig specifies that it can be an undefined

export type MeilisearchConfig = {
  host: string
  apiKey: string
  indexUid: string
  fieldsToIndex?: string
}

And typescript does not infer that the variable was already set to string in config so it raises an error

Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
Type 'undefined' is not assignable to type 'string'.

@@ -48,6 +48,7 @@ program
'-a, --api-key <api-key>',
'The Meilisearch API key with permission to perform actions on indexes. Both the private key and the master key are valid choices but we strongly recommend using the private key for security purposes.'
)
.option('-f, --fields-to-index <fields-to-index>', 'test')
Copy link
Contributor

Choose a reason for hiding this comment

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

What is test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops

Copy link
Contributor

@alallema alallema left a comment

Choose a reason for hiding this comment

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

Well Done 🚀
LGTM! ✨

@bidoubiwa
Copy link
Contributor Author

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Oct 26, 2022

@meili-bors meili-bors bot merged commit 67bfcd3 into main Oct 26, 2022
@meili-bors meili-bors bot deleted the add_fields_to_index_as_cli_flag branch October 26, 2022 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limiting fields for import script
2 participants