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

feat: use browserify to bundle hoodie client (#532) #538

Merged
merged 1 commit into from
Jul 25, 2016

Conversation

sdvg
Copy link
Contributor

@sdvg sdvg commented Jul 24, 2016

This makes the /hoodie/client.js request use Browserify to bundle an up to date version of @hoodie/account-client instead of delivering the prebuilt version in /dist.

For more details see issue #532.

@gr2m
Copy link
Member

gr2m commented Jul 24, 2016

LGTM, great work 👍. I’ve checked it locally with hoodie-app-tracker and its integration tests and they all pass.

Ping @hoodiehq/maintainers for 2nd 👀

@HipsterBrown
Copy link

Is this a breaking change since it changes the hoodieClientPath?

Otherwise, LGTM 👍

@varjmes
Copy link
Contributor

varjmes commented Jul 25, 2016

Once @HipsterBrown has his question answered, this LGTM. Thanks @sdvg! ✨

@rogeriochaves
Copy link
Member

LGTM

@gr2m
Copy link
Member

gr2m commented Jul 25, 2016

@HipsterBrown @Charlotteis

This change is part of the browserify on demand

-  var hoodieClientPath = path.join(hoodieClientModulePath, 'dist/hoodie.js')
+  var hoodieClientPath = path.join(hoodieClientModulePath, 'index.js')

Instead of using the pre-built file at dist/hoodie.js, we now need the main file of @hoodie/client which is index.js. So it’s all good, it’s only the input path. The output path remains the same.

Thanks for your reviews everyone, this is great 👏 I’ll go ahead and merge it now

@gr2m gr2m merged commit d715624 into master Jul 25, 2016
@gr2m gr2m removed the in progress label Jul 25, 2016
@gr2m gr2m deleted the 532-browserify-client branch July 25, 2016 14:59
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.

6 participants