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

chore(examples): update demos to use instantsearch from repo #4030

Merged
merged 6 commits into from
Aug 21, 2019

Conversation

yannickcr
Copy link
Contributor

@yannickcr yannickcr commented Aug 9, 2019

Summary

To improve InstantSearch releases reliability we plan to add some End-2-End tests in addition to our existing unit test suite.

Since we'll use the e-commerce demo to run our tests then we need to update it to use the instantsearch.js build from the current branch.

To achieve this, this PR makes the following changes:

  • Remove instantsearch.js from dependencies
  • Alias instantsearch.js to use the build in the parent directory (using Parcel aliases and TypeScript path mapping)
  • Update website:build command to build instantsearch before the demos

@Haroenv
Copy link
Contributor

Haroenv commented Aug 9, 2019

Seems good, but should we not do that for all examples? Downside is now that it won’t work standalone in csb anymore. I’d really rather have a purpose-built example for the tests

@yannickcr
Copy link
Contributor Author

Seems good, but should we not do that for all examples?

Mmm, I don't think we'll run some tests on them (maybe on the new media demo but it is not merged yet #3915) but I can update them to stay consistent 🙂

Downside is now that it won’t work standalone in csb anymore. I’d really rather have a purpose-built example for the tests

Is it a problem? I never saw these demos used in csb.
I think we agreed to use this one for now to go faster but to build some test specific demos later on.

@yannickcr yannickcr changed the title chore(examples): update e-commerce demo to use instantsearch from repo chore(examples): update demos to use instantsearch from repo Aug 9, 2019
@francoischalifour
Copy link
Member

I don't remember if we agreed to run it on the actual example. I thought the reasoning was that we should create a minimal example (e.g. no Google Fonts, no image header to speed up the page load). We could add these minimal examples in the e2e/ folder. It's indeed nice to be able to run the example in CodeSandbox (some people asked for it already).

WDYT?

@yannickcr
Copy link
Contributor Author

I don't remember if we agreed to run it on the actual example. I thought the reasoning was that we should create a minimal example (e.g. no Google Fonts, no image header to speed up the page load). We could add these minimal examples in the e2e/ folder.

Yes, but it was planned to do this in a second time since already having a running test suite on the existing examples was way faster to set up and already bring us a lot of value to improve InstantSearch reliability.

It's indeed nice to be able to run the example in CodeSandbox (some people asked for it already).

New examples (like the e-commerce demo) are already not runnable in CodeSansbox anyway, no? Or I am missing something here?

@Haroenv
Copy link
Contributor

Haroenv commented Aug 12, 2019

@yannickcr
Copy link
Contributor Author

yannickcr commented Aug 12, 2019

Ok, I missed that CodeSandbox runs the parcel build.

Still not convinced that keeping this "feature" worth the hassle tho.

@yannickcr
Copy link
Contributor Author

As we decided IRL last week, to go faster we'll use the existing demos for now and see in the future if some dedicated demos are required.

Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Netlify is still stuck though.

@yannickcr yannickcr merged commit 6994c77 into develop Aug 21, 2019
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.

3 participants