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

Fix SSR issues with SortableJS 1.10.0 #723

Merged
merged 2 commits into from Sep 21, 2019
Merged

Fix SSR issues with SortableJS 1.10.0 #723

merged 2 commits into from Sep 21, 2019

Conversation

Hugome
Copy link
Contributor

@Hugome Hugome commented Sep 17, 2019

SortableJS 1.10.0 has been release 2 days ago and break some SSR things.
Before 1.10.0 : Every "navigator.useragent"/"typeof window" check was not done on the module import.
Now : When you import sortable it does some client only check :
image
So the only solution is too import Sortable only on the client side (Like in this PR).
Is this solution really ugly ? I'm not 100 % sure, but the only place this plugin use the Sortable constructor is on the mounted hook, and this hook in SSR is only called on client side (https://ssr.vuejs.org/guide/universal.html) so it will have the module included.

@David-Desmaisons
Copy link
Member

Thanks for subimiting this PR.
Did you test this fix to make there is no ther regression in SSR scenario?

@Hugome
Copy link
Contributor Author

Hugome commented Sep 19, 2019

After a little digging i find a problem with this solution, the good old commonjs/es module, import/require black magic.

1st) I forgot the "default" in the require, resolved
2nd) This "default" break tests, resolved by changing the mock to be an esmodule

I try in my application and it seem good
(This fix the new #724 issue)

@David-Desmaisons
Copy link
Member

OK, thanks for this. I will take a look today and try to merge in the short term.

@David-Desmaisons David-Desmaisons mentioned this pull request Sep 21, 2019
@David-Desmaisons
Copy link
Member

I made a small chnage to simplify your fix in PR #726

@David-Desmaisons David-Desmaisons merged commit eba8730 into SortableJS:master Sep 21, 2019
@David-Desmaisons
Copy link
Member

Thanks @Hugome

@transtone
Copy link

not solve: #732

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.

4 participants