Skip to content

Conversation

@a11rew
Copy link
Contributor

@a11rew a11rew commented Oct 23, 2022

Pull Request

Related issue

Fixes #765

What does this PR do?

  • Adds support for passing function as API key
  • Documents this feature in the README
  • Adds validation for instantMeiliSearch parameters
    • Ensures hostUrl is a string and throws an error if otherwise
    • Ensures apiKey is a string or a function that returns a string, throws errors if otherwise
  • Adds tests for instantMeiliSearch parameters validation

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@bidoubiwa
Copy link
Contributor

Hello @a11rew,
Thank you very much for contributing to Meilisearch ❤️.
However, I am not available on the weekend, but I will be back on Monday 😊.

PS: This message was sent automatically!

Comment on lines 37 to 61
// Validate parameters
if (typeof hostUrl !== 'string') {
throw new TypeError(
'Provided hostUrl value (1st parameter) is not a string, expected string'
)
}

if (typeof apiKey !== 'string' && typeof apiKey !== 'function') {
throw new TypeError(
'Provided apiKey value (2nd parameter) is not a string or a function, expected string or function'
)
}

// If apiKey is function, call it to get the apiKey
if (typeof apiKey === 'function') {
const apiKeyFnValue = apiKey()
if (typeof apiKeyFnValue !== 'string') {
throw new TypeError(
'Provided apiKey function (2nd parameter) did not return a string, expected string'
)
}

// Replace apiKey with the value returned by the function
apiKey = apiKeyFnValue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be nice to move this to a validate.ts file to avoid to much code in the client constructor. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that would work, moving lines 37-48 into a separate function in a different file.

I'll be leaving the call that resolves the function (lines 50-61) to a string here however so the validate function isn't returning values, just throwing errors on invalid values. What do you think, is that fine?

Copy link
Contributor

@bidoubiwa bidoubiwa Oct 24, 2022

Choose a reason for hiding this comment

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

What do you think about moving it to an apart function not related to validation?

function getApiKey(apiKey: string | () => string) {
     // If apiKey is function, call it to get the apiKey
  if (typeof apiKey === 'function') {
    const apiKeyFnValue = apiKey()
    if (typeof apiKeyFnValue !== 'string') {
      throw new TypeError(
        'Provided apiKey function (2nd parameter) did not return a string, expected string'
      )
    }

    return apiKeyFnValue
  }
  return apiKey
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it out of the constructor, albeit in the same file since it throws an error specific only to the client constructor

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Thanks for this amazing PR 🔥

a11rew and others added 3 commits October 24, 2022 12:37
bidoubiwa
bidoubiwa previously approved these changes Oct 25, 2022
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@bidoubiwa bidoubiwa added the enhancement New feature or request label Oct 25, 2022
@bidoubiwa
Copy link
Contributor

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Oct 25, 2022

@meili-bors meili-bors bot merged commit df4714f into meilisearch:main Oct 25, 2022
@bidoubiwa
Copy link
Contributor

This message is sent automatically

Thanks again for contributing to Meilisearch ❤️
If you are participating in Hacktoberfest, and you would like to receive some gift from Meilisearch too, please complete this form.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow passed API key to be a callback function

2 participants