Add Browsersync for mobile development testing #712

t-kelly · 2018-08-24T15:11:31Z

What are you trying to accomplish with this PR?

Fixes #594

Adds Browsersync so that your development store with assets being served from your local asset server is accessible to other devices on your local network.

TODO

Validate if the Browsersync UI is working. ~~I have tried to load it on my dev machine but the connection is failing for some reason...~~ UPDATE: Looks to be caused by Https and open ui > panel empty (no url) BrowserSync/browser-sync#1152

dan-gamble · 2018-08-28T09:05:11Z

I've given this a run locally and it seems to work well on my desktop like it usually was.

I can access the site via the browser sync url on my phone and that works but i don't get the assets. The error i'm getting is a https://localhost:3001/theme.index.js can't be found. Should this be using the browser sync ip as opposed to localhost?

https://i.imgur.com/0m1wnSu.png

dan-gamble · 2018-08-28T09:09:39Z

Just to add to this if i update my local domain to be my internal IP i.e. domain: 'https://192.168.0.28', then it works 👍

dan-gamble · 2018-08-28T15:05:07Z

To add to this. In IE11 everything seems to be working okay but the scripts aren't being loaded. The line is there but they don't seem to appear in the network tab so not quite sure what's going on. Could be a defer issue that isn't necessarily related to this.

el-rotny · 2018-08-29T08:32:00Z

Good to see this being close. Will try to lend a hand and test tomorrow and see why the UI isnt working.

On Fri, Aug 24, 2018 at 8:11 AM Thomas Kelly ***@***.***> wrote: [image: image 1] <https://user-images.githubusercontent.com/4837696/44592206-ed675680-a78d-11e8-8c04-1df864a86c60.png> What are you trying to accomplish with this PR? Fixes #594 (comment) <#594 (comment)> Adds Browsersync so that your development store with assets being served from your local asset server is accessible to other devices on your local network. TODO - Validate if the Browsersync UI is working. I have tried to load it on my dev machine but the connection is failing for some reason... ------------------------------ You can view, comment on, or merge this pull request online at: #712 Commit Summary - Add Browsersync for mobile development testing File Changes - *M* packages/slate-tools/cli/commands/start.js <https://github.com/Shopify/slate/pull/712/files#diff-0> (73) - *M* packages/slate-tools/package.json <https://github.com/Shopify/slate/pull/712/files#diff-1> (2) - *M* packages/slate-tools/slate-tools.config.js <https://github.com/Shopify/slate/pull/712/files#diff-2> (2) - *R* packages/slate-tools/tools/asset-server/app.js <https://github.com/Shopify/slate/pull/712/files#diff-3> (0) - *R* packages/slate-tools/tools/asset-server/client.js <https://github.com/Shopify/slate/pull/712/files#diff-4> (4) - *A* packages/slate-tools/tools/asset-server/index.js <https://github.com/Shopify/slate/pull/712/files#diff-5> (94) - *M* packages/slate-tools/tools/dev-server/index.js <https://github.com/Shopify/slate/pull/712/files#diff-6> (112) - *D* packages/slate-tools/tools/dev-server/ssl/index.js <https://github.com/Shopify/slate/pull/712/files#diff-7> (14) - *A* packages/slate-tools/tools/utilities/index.js <https://github.com/Shopify/slate/pull/712/files#diff-8> (24) - *R* packages/slate-tools/tools/utilities/server.pem <https://github.com/Shopify/slate/pull/712/files#diff-9> (0) - *M* yarn.lock <https://github.com/Shopify/slate/pull/712/files#diff-10> (557) Patch Links: - https://github.com/Shopify/slate/pull/712.patch - https://github.com/Shopify/slate/pull/712.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#712>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACGRE48bzctC6CF9Fao7NatZ4sZN-p86ks5uUBengaJpZM4WLhvb> .

-- Sent from my iPhone. Excuse the typos.

t-kelly · 2018-08-29T10:11:29Z

Should this be using the browser sync ip as opposed to localhost?

Nope. localhost:3001 should work fine.

In IE11 everything seems to be working okay but the scripts aren't being loaded.

I'll take a look.

Just to add to this if i update my local domain to be my internal IP then it works:

Shouldn't need to do this.

@dan-gamble are you accessing your dev store externally via the myshopify.com URL or via your IP:Port (in your case https://192.168.0.28:3000). You should be using the IP:port as it will make sure localhost:3001 is pointing to the localhost on your dev machine.

t-kelly · 2018-08-29T10:37:29Z

If I add:

  socket: {
    domain: 'https://localhost:3000',
  },

to the browswersync config, the UI loads on 192.168.x.x:3002 but not localhost:3002. Going open a new issue in Browsersync repo.

dan-gamble · 2018-08-29T11:30:27Z

Hey @t-kelly. I was accessing the site via 192.168.0.28:3000 in my virtual machine and on my iPhone in both cases and it was blanking out. Let me give it another quick try to see if i was doing something silly.

dan-gamble · 2018-08-29T11:40:39Z

Hi Thomas,

I should've clarified my findings a bit more. The https://192.168.0.28:3000 works totally fine on yarn start without the domain bit. But that points to the "live" theme. If i add ?preview_theme_id=xxxxx so it's https://192.168.0.28:3000?preview_theme_id=xxxxx then the assets no long work unless i have the domain present in slate.config.js.

IE11 also works totally fine until the ?preview_theme_id=xxxxx is added where then even with the domain it doesn't seem to load assets.

t-kelly · 2018-08-29T14:24:03Z

@dan-gamble I was hoping preview_theme_id was being passed in the proxyReq setting I setup. Will validate what's up there.

t-kelly · 2018-08-31T18:57:02Z

Okay -- I've done some more work. Slate will now default to using your local IP address to reference assets instead of localhost. This will enable developers to access their dev site on their phone without having to deal with the complexities of SSL + localhost. Note, you will still need to visit the Asset Server URL at least once to tell your browser to trust it.

I now do a check when firing up the dev environment to see if your local IP is public or private. If it is private we can use it securely for external testing. If it is public, the CLI recommends you continue with external testing disabled because it's not a good idea to post your public IP on a live Shopify store.

Final test to see if it works? I'll do a code cleanup after because I've made a mess of things now :/

dan-gamble · 2018-09-01T07:01:54Z

I'll give this a go Monday or if i find time this weekend while moving i'll give it a quick run :)

dan-gamble · 2018-09-03T10:36:54Z

packages/slate-tools/cli/commands/start.js

 const env = require('@shopify/slate-env');
 const {event} = require('@shopify/slate-analytics');

 const promptContinueIfPublishedTheme = require('../prompts/continue-if-published-theme');
 const promptSkipSettingsData = require('../prompts/skip-settings-data');
+const promptDisableExternalTesting = require('../prompts/confim-private-ip');


There's a typo on this file (confim => confirm)

dan-gamble · 2018-09-03T10:44:11Z

packages/slate-tools/package.json

@@ -56,8 +57,10 @@
    "jest": "22.4.2",
    "minimatch": "^3.0.4",
    "minimist": "1.2.0",
+    "node-ip": "^0.1.2",


I think this is meant to be "ip": "^1.1.5", or the prompts/confim-private-ip.js needs to have it's require() change.

Yes! Got mixed up there. Thanks.

dan-gamble · 2018-09-03T11:06:26Z

Can't really fully test this on IE11 as i can't work out how to visit a :3001 address and accept the cert. Any JS file i go to prompts a download so i can't visit the page i want. No CSS files exist in development, images seem to be uploaded to CDN. Do you know of an asset i can visit to get this working?

t-kelly · 2018-09-04T14:25:36Z

@dan-gamble I gave IE11 a crack and everything seemed to work fine after visiting https://x.x.x.x:3000?preview_theme_id=xxxxxxxx. I didn't need to accept or navigate to any assets on the asset server port (3001).

dan-gamble · 2018-09-04T15:12:45Z

I'll give it another go, awesome PR though @t-kelly. Thanks a ton for getting this in and prioritising it.

lmartins · 2018-09-12T14:32:50Z

Thanks so much for this feature @t-kelly this will be tremendously useful.
On IE11 and Edge, I still have issues where the assets are not loaded. I've tried the trick of opening an asset link directly, but for those browsers nothing seems to works and those requests are always rejected.
So excited to have this feature available in Slate! Thank you!

Update: One thing I'm noticing with beta 7, is that the refresh actually takes a few seconds more. While before it was pretty much instant, now it takes anything between 3 to 6 seconds for the browser to receive the updates.

lock · 2018-10-26T15:35:56Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

t-kelly added enhancement v1.x labels Aug 24, 2018

t-kelly self-assigned this Aug 24, 2018

t-kelly requested a review from bertiful August 24, 2018 15:11

Add Browsersync for mobile development testing

998dd6d

t-kelly force-pushed the add-browsersync branch from f6be348 to 998dd6d Compare August 24, 2018 15:22

Default to using local IP address instead of localhost

f0bf67e

dan-gamble reviewed Sep 3, 2018

View reviewed changes

t-kelly force-pushed the add-browsersync branch from 8d13767 to 0ba97b6 Compare September 4, 2018 14:23

Adjustments from PR feedback and CI

25810f0

t-kelly force-pushed the add-browsersync branch from 0ba97b6 to 25810f0 Compare September 4, 2018 14:55

t-kelly merged commit 6740b7e into master Sep 4, 2018

t-kelly deleted the add-browsersync branch September 4, 2018 15:00

This was referenced Sep 5, 2018

Increment port if Slate is already running #553

Closed

BrowserSync SSL certificate #726

Closed

t-kelly mentioned this pull request Sep 6, 2018

Previewing broken after setting up Localhost SSL Certificate #727

Closed

lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Browsersync for mobile development testing #712

Add Browsersync for mobile development testing #712

t-kelly commented Aug 24, 2018 •

edited

Loading

dan-gamble commented Aug 28, 2018

dan-gamble commented Aug 28, 2018

dan-gamble commented Aug 28, 2018

el-rotny commented Aug 29, 2018 via email

t-kelly commented Aug 29, 2018

t-kelly commented Aug 29, 2018

dan-gamble commented Aug 29, 2018

dan-gamble commented Aug 29, 2018

t-kelly commented Aug 29, 2018

t-kelly commented Aug 31, 2018

dan-gamble commented Sep 1, 2018

dan-gamble Sep 3, 2018

dan-gamble Sep 3, 2018

t-kelly Sep 4, 2018

dan-gamble commented Sep 3, 2018

t-kelly commented Sep 4, 2018

dan-gamble commented Sep 4, 2018

lmartins commented Sep 12, 2018 •

edited

Loading

lock bot commented Oct 26, 2018

Add Browsersync for mobile development testing #712

Add Browsersync for mobile development testing #712

Conversation

t-kelly commented Aug 24, 2018 • edited Loading

What are you trying to accomplish with this PR?

TODO

dan-gamble commented Aug 28, 2018

dan-gamble commented Aug 28, 2018

dan-gamble commented Aug 28, 2018

el-rotny commented Aug 29, 2018 via email

t-kelly commented Aug 29, 2018

t-kelly commented Aug 29, 2018

dan-gamble commented Aug 29, 2018

dan-gamble commented Aug 29, 2018

t-kelly commented Aug 29, 2018

t-kelly commented Aug 31, 2018

dan-gamble commented Sep 1, 2018

dan-gamble Sep 3, 2018

Choose a reason for hiding this comment

dan-gamble Sep 3, 2018

Choose a reason for hiding this comment

t-kelly Sep 4, 2018

Choose a reason for hiding this comment

dan-gamble commented Sep 3, 2018

t-kelly commented Sep 4, 2018

dan-gamble commented Sep 4, 2018

lmartins commented Sep 12, 2018 • edited Loading

lock bot commented Oct 26, 2018

t-kelly commented Aug 24, 2018 •

edited

Loading

lmartins commented Sep 12, 2018 •

edited

Loading