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(v3): remove URLSync #3143

Merged
merged 7 commits into from
Sep 26, 2018
Merged

chore(v3): remove URLSync #3143

merged 7 commits into from
Sep 26, 2018

Conversation

bobylito
Copy link
Contributor

@bobylito bobylito commented Sep 24, 2018

This PR removes the URLSync.

This PR depends on #3148 to be fixed.

@algobot
Copy link
Contributor

algobot commented Sep 24, 2018

Deploy preview for algolia-instantsearch ready!

Built with commit 26121ec

https://deploy-preview-3143--algolia-instantsearch.netlify.com

@@ -226,7 +206,7 @@ class InstantSearch extends EventEmitter {
// We don't want to re-add URlSync `getConfiguration` widget
// it can throw errors since it may re-add SearchParameters about something unmounted
this.searchParameters = this.widgets
.filter(w => w.constructor.name !== 'URLSync')
.filter(w => w.constructor.name !== 'URLSync') // TODO: check the routing for this
Copy link
Member

Choose a reason for hiding this comment

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

Can we check this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no issue as far as I can test with the current behavior. I'm getting rid of it.

@@ -205,9 +205,9 @@ class InstantSearch extends EventEmitter {
if (nextState) {
// We don't want to re-add URlSync `getConfiguration` widget
// it can throw errors since it may re-add SearchParameters about something unmounted
this.searchParameters = this.widgets
.filter(w => w.constructor.name !== 'URLSync') // TODO: check the routing for this
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to let you know, we can't use the name attribute of function or constructor. Once Uglify pass on the code all values are renamed and the constructor name is changed (except with an Uglify option that is by default to false, see keep_fnames ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know thanks. It might have never worked in production then for the URLSync.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep indeed.

@bobylito
Copy link
Contributor Author

The doc is now updated

@Haroenv
Copy link
Contributor

Haroenv commented Sep 26, 2018

Linting is failign in this PR :)

@bobylito
Copy link
Contributor Author

Linting is failign in this PR :)

Linting never sleeps :)

@bobylito bobylito merged commit cdea3ab into feat/3.0 Sep 26, 2018
@bobylito bobylito deleted the feat/3.0-rm-urlSync branch September 26, 2018 15:57
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.

5 participants