Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

@eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented May 24, 2021

Summary

This PR adds a template to create a widget in React InstantSearch.

// import { defineConfig } from 'vite' -> fails when type:module in package.json
// it seems related to https://github.com/vitejs/vite/issues/1560
// so far this is the workaround.
const { defineConfig } = vite;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI

placeholder?: string;
};

export const {{ pascalCaseName }}: ElementType<WidgetParams> =
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'm not sure ElementType is the right one, but anything else like ReactElement, Component, etc didn't work.

By "didn't work", I mean TS gave me error messages like

CleanShot 2021-05-25 at 16 48 25@2x

Copy link
Contributor

Choose a reason for hiding this comment

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

as far as I can tell ElementType is correct

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 also be ReactNode, did you try that?

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, ReactNode, ReactElement, all didn't work

// TODO: this is the object returned by `getProvidedProps` from the connector.
};

export const {{ pascalCaseName }}Component = ({}: Props) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do we want to do? Here the Props is loose. It's not connected with the return of getProvidedProps from the connector. Do we want to type this template at that level? Or this is okay enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

can it not be imported from the connector file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return type of getProvidedProps won't include refine function, but the Props here in the component will. And we might want to specify in the connector as a generic, what kind of type the refine function will take. So these thoughts kept leading to another, and I wonder maybe the current version is incomplete but simple to start with.

Copy link
Contributor

Choose a reason for hiding this comment

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

could it be ReturnType & { refine } or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CleanShot 2021-06-08 at 16 44 05@2x

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.

@eunjae-lee eunjae-lee requested review from a team, Haroenv, francoischalifour and tkrugg and removed request for a team May 25, 2021 15:18
@eunjae-lee eunjae-lee marked this pull request as ready for review June 1, 2021 15:08
"instantsearch-widget",
"instantsearch",
"react-instantsearch",
"react-instantsearch-widget-{{ packageName }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

the package name includes the scope, it should probably be kebabCaseName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point 2ea1b17

widgetType: `${organization}.${name}`,
camelCaseName: camelCase(name),
pascalCaseName: capitalize(camelCase(name)),
kebabCaseName: kebabCase(name),
Copy link
Contributor

Choose a reason for hiding this comment

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

the name itself should probably already be kebab case, but since you're converting, you should also use it for wigdetType / packageName, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I just checked and we're using validate-npm-package-name which validates name for us. It's not necessarily kebab case, but should be fine as a keyword as-is.

1603463

b37ad94

@eunjae-lee eunjae-lee requested a review from Haroenv June 10, 2021 09:28
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

I think this is good as a starting point, improvements in the questions asked can be done for both templates at the same time

@eunjae-lee eunjae-lee merged commit cf2c81b into master Jun 10, 2021
@eunjae-lee eunjae-lee deleted the feat/custom-widget-react branch June 10, 2021 15:23
aymeric-giraudet pushed a commit to algolia/instantsearch that referenced this pull request Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants