Re-use an existing browser tab with with the same URL when launching `frontity dev` #374

michalczaplinski · 2020-04-14T00:57:11Z

What:

When running frontity dev, a new browser tab is reopened every time the command is run.

Now, frontity dev will attempt to re-use the already open browser tab and switch to it instead of opening a new tab, So, if you already have a tab with http://localhost:3000 open in chrome, we just switch to it. This stops the script from opening many tabs with the same location.

The implementation is a slightly modified version of create-react-app implementation https://github.com/facebook/create-react-app/blob/master/packages/react-dev-utils/openBrowser.js therefore I have retained the original MIT license in the file.

Additionally, I have pinned the react-helmet-async dependency to 1.0.4 because in the latest release they have broken the types in index.d.ts and it causes an error on npm install in our repository. staylor/react-helmet-async#91

Tasks:

Unrelated:

~~Update starter themes~~
~~Update other packages~~
~~Community discussions~~
~~Documentation~~
~~Unit tests~~
~~End to end tests~~
~~TypeScript tests~~

It has a wrong index.d.ts and thus breaking frontity build

… one if it is already open

changeset-bot · 2020-04-14T00:57:15Z

🦋 Changeset is good to go

Latest commit: 2df3446

We got this.

This PR includes changesets to release 1 package

Name	Type
@frontity/core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

michalczaplinski · 2020-04-14T01:02:51Z

I think this is the kind of change that is a great candidate for e2e testing, but I'm happy to hear other ideas on how you'd test it.

luisherranz · 2020-04-14T06:24:08Z

Additionally, I have pinned the react-helmet-async dependency to 1.0.4 because in the latest release they have broken the types in index.d.ts and it causes an error on npm install in our repository. staylor/react-helmet-async#91

You should not have used a single PR for two different things because now I can't approve the react-helmet-async fix right away.

This reverts commit 3e31d8e.

luisherranz · 2020-04-14T06:47:10Z

I think this is the kind of change that is a great candidate for e2e testing

Sure. What are your ideas?

luisherranz · 2020-04-14T15:23:02Z

Is this ready for review or are you going to add tests first?

michalczaplinski · 2020-04-14T15:31:48Z

Sure. What are your ideas?

So, I don't think that you can test the actual functionality of this change (that it does not open a new tab but rather switch to the existing tab). Or, at least, it would be a bit absurd to try to test it this way...

Could we not try to run the cypress e2e tests against a frontity dev as well as a production bundle? This way, we can at least confirm that frontity dev is not broken

Right now, we're only building and running frontity serve:

frontity/.github/workflows/e2e-tests.yml

Line 34 in 4d5bdd4

start: npm run e2e:prod:${{ matrix.ci_node_index }}

which is calling:

frontity/package.json

Lines 7 to 9 in 107d354

    
           "e2e:prod:all": "cd e2e/project/ && npx frontity build && npx frontity serve --port 3001", 
        
           "e2e:prod:module": "cd e2e/project/ && npx frontity build --target module && npx frontity serve --port 3001", 
        
           "e2e:prod:es5": "cd e2e/project/ && npx frontity build --target es5 && npx frontity serve --port 3001",

luisherranz · 2020-04-15T07:01:06Z

Sure, good idea. Maybe we can even add a write to a file to make sure HMR is working.

Should we add a frontity dev e2e test to the testing roadmap? It doesn't feel like it belongs here, right?

luisherranz · 2020-04-15T07:04:44Z

Have you tested this in Ubuntu and Windows?

michalczaplinski · 2020-04-15T17:11:21Z

Have you tested this in Ubuntu and Windows?

I haven't because I only have MacOS and no space for a VM on my machine : ( Do you know a cloud VM service that I could use or are you able to test it yourself?

Should we add a frontity dev e2e test to the testing roadmap? It doesn't feel like it belongs here, right?

I'm not sure. I think we could just add an extra test to launch the e2e test with `frontity dev` just to make sure "it works". There is already an npm script for it: https://github.com/frontity/frontity/blob/1c68016be047f890c0ae48a7dd4a5b507e3609bc/e2e/package.json#L9 Can I just do it? :)

luisherranz · 2020-04-16T06:06:44Z

Do you know a cloud VM service that I could use or are you able to test it
yourself?

Maybe with https://crossbrowsertesting.com/ or https://www.browserstack.com/?

Can I just do it? :)

Sure.

luisherranz

To be consistent between args and ENV variables we need to change the browser arg of the dev command too: https://github.com/frontity/frontity/blob/dev/packages/core/src/scripts/dev.ts#L21

Also, to be consistent with the work of #262 you should rename BROWSER to FRONTITY_BROWSER.

luisherranz · 2020-04-16T06:17:54Z

packages/core/src/scripts/utils/open-browser.ts

+    case Actions.NONE:
+      // Special case: BROWSER="none" will prevent opening completely.
+      return false;
+    case Actions.SCRIPT:


What is this for?

@luisherranz I don't exactly know as this was copied 1:1 from create-react-app.

By just looking at the code, it seems that if the value of the BROWSER environment variable is a .js script, it executes this script instead of launching a browser. I'm not sure if this is a workaround for some edge case, etc. so I would just keep this code as is.

I wouldn't like to add code to the framework without understanding it so please check it out before we merge.

Also, if this covers some use cases, it needs to go to the documentation.

Looks like this is just an advanced feature, in case you want to run a js script that eventually launches a browser in order to do some initialization, etc. There are some more details here: https://create-react-app.dev/docs/advanced-configuration/

Maybe this could be useful for automated testing with cypress? 🤔

michalczaplinski · 2020-04-16T16:40:24Z

To be consistent between args and ENV variables we need to change the browser arg of the dev command too: https://github.com/frontity/frontity/blob/dev/packages/core/src/scripts/dev.ts#L21

I'm not sure what you mean, or perhaps it's a misunderstanding. That specific option (openBrowser) refers to whether a browser window should be opened or not. The BROWSER env variable holds the path to the default browser, so those are 2 different things if I understand you correctly.

Also, to be consistent with the work of #262 you should rename BROWSER to FRONTITY_BROWSER.

The BROWSER variable is a system-wide setting across different operating systems (and especially on linux) and not a Frontity-specific option, so I think we should just keep it as BROWSER.

michalczaplinski · 2020-04-16T16:43:24Z

Also, should I wait until I add the frontity dev e2e test or can we merge it beforehand?

luisherranz · 2020-04-17T06:10:48Z

The BROWSER env variable holds the path to the default browser, so those are 2 different things if I understand you correctly.

Looking at the code I see that you can use BROWSER=none which is basically --openBrowser=false so they overlap and it should be only one or it is going to be confusing.

If we go with BROWSER instead of --openBrowser we need to remove --openBrowser and create a --browser that matches BROWSER.

The BROWSER variable is a system-wide setting across different operating systems (and especially on linux) and not a Frontity-specific option, so I think we should just keep it as BROWSER.

Ok. For consistency with the rest of our ENV variables, what about accepting both BROWSER and FRONTITY_BROWSER? Does it make sense?

luisherranz · 2020-04-17T06:11:28Z

should I wait until I add the frontity dev e2e test

I don't think it is related so whatever you want.

packages/core/src/scripts/utils/open-browser.ts

michalczaplinski · 2020-04-23T23:47:06Z

Looking at the code I see that you can use BROWSER=none which is basically --openBrowser=false so they overlap and it should be only one or it is going to be confusing.

If we go with BROWSER instead of --openBrowser we need to remove --openBrowser and create a --browser that matches BROWSER.

Right, I see what you mean. I had a look and the current CLI flag is actually --dont-open-browser 😄

frontity/packages/frontity/src/cli/index.ts

Line 70 in 107d354

"--dont-open-browser",

So, I think it's fine if we just keep the BROWSER env variable as is and then the matrix of options is:

BROWSER=none, whatever other flags passed - we dont open the browser
BROWSER=something, --dont-open-browser - also dont open browser
BROWSER=something, no CLI flags - open the browser
no env variables or CLI flags - open the browser

Ok. For consistency with the rest of our ENV variables, what about accepting both BROWSER and FRONTITY_BROWSER? Does it make sense?

With respect to that I don't really have a strong opinion, but one argument in favour of just using the BROWSER variable is that some users might be already familiar with it. I don't see how anybody would be getting confused even if this one variable is not consistent in naming with the rest of variables in frontity 🙂. I think accepting 2 environmental variables that serve the same purpose would be more confusing, in my opinion!

michalczaplinski · 2020-04-23T23:57:06Z

@luisherranz Could you please check if this works on windows when running frontity dev ? 🙏

I've signed up for browserstack but as far as I can see I only use a browser to look at websites and I cannot use a full windows environment, unfortunately. I've tested it on MacOS and works as expected. On windows it should fall back to just opening a new tab.

Let me know if there's anything else stopping this from merging 🙂

…open-browser-tab

codecov · 2020-05-29T12:58:36Z

Codecov Report

Merging #374 into dev will not change coverage.
The diff coverage is n/a.

luisherranz · 2020-05-29T18:16:44Z

I've finally managed to run our monorepo in Windows and try this PR, but it's not working: it opens a new tab each time you run npx frontity dev.

I don't know if I'm missing something. I'm off for today but we can take a look together next week.

luisherranz · 2020-05-29T18:17:26Z

@DAreRodz can you test it in Ubuntu?

DAreRodz · 2020-05-29T20:52:34Z

@DAreRodz can you test it in Ubuntu?

I've just tested it and the dev command always opens a new tab. 😕

Maybe it is because the create-react-app solution only works for OSX? Each time I run npm start in a create-react-app project also opens a new tab.

DAreRodz · 2020-05-29T21:22:08Z

Well, it is. Looking at the code, there is only one specific case when the tab can be reused, and is this one:

frontity/packages/core/src/scripts/utils/open-browser.ts

Lines 66 to 69 in 35966b4

    
           // If we're on OS X, the user hasn't specifically 
        
           // requested a different browser, we can try opening 
        
           // Chrome with AppleScript. This lets us reuse an 
        
           // existing tab when possible instead of creating a new one.

The rest of the covered cases are these (and use open as we currently do):

frontity/packages/core/src/scripts/utils/open-browser.ts

Lines 108 to 129 in 35966b4

    
           // Another special case: on OS X, check if BROWSER has been set to "open". 
        
           // In this case, instead of passing `open` to `opn` (which won't work), 
        
           // just ignore it (thus ensuring the intended behavior, i.e. opening the system browser): 
        
           // https://github.com/facebook/create-react-app/pull/1690#issuecomment-283518768 
        
           if (process.platform === "darwin" && browser === "open") { 
        
             browser = undefined; 
        
           } 
        
           // If there are arguments, they must be passed as array with the browser 
        
           if (typeof browser === "string" && args.length > 0) { 
        
             browser = [browser].concat(args); 
        
           } 
        
           // Fallback to open 
        
           // (It will always open new tab) 
        
           try { 
        
             const options = { app: browser, wait: false, url: true }; 
        
             open(url, options).catch(() => {}); // Prevent `unhandledRejection` error. 
        
             return true; 
        
           } catch (err) { 
        
             return false; 
        
           }

luisherranz · 2020-05-30T08:52:53Z

I see, thanks @DAreRodz. @michalczaplinski can you confirm that? Is this supposed to only work on OSX?

michalczaplinski · 2020-05-30T17:46:28Z

I see, thanks @DAreRodz. @michalczaplinski can you confirm that? Is this supposed to only work on OSX?

That's correct! And I'm sorry -that should have been more clear from the beginning.

By "works on windows & linux" I meant that it doesn't break anything, but the actual behaviour of preserving the tab only works on MacOS.

packages/core/package.json

.changeset/rare-mayflies-clap.md

…open-browser-tab

luisherranz

Thanks @michalczaplinski. I'm really sorry it took me so long to review this. Let's merge it now 🙂

michalczaplinski added 3 commits April 13, 2020 19:44

frontity/frontity: Pin the react-async-helmet dependency

3e31d8e

It has a wrong index.d.ts and thus breaking frontity build

core: Make frontity dev open the current browser tab instead of a new…

83cddb1

… one if it is already open

Add changesets

5ada3cb

michalczaplinski self-assigned this Apr 14, 2020

michalczaplinski requested a review from luisherranz April 14, 2020 01:02

michalczaplinski changed the title ~~Open browser tab~~ Re-use an existing browser tab with with the same URL when launching frontity dev Apr 14, 2020

luisherranz mentioned this pull request Apr 14, 2020

Fix react helmet async update #375

Merged

10 tasks

luisherranz added 2 commits April 14, 2020 08:45

Revert "frontity/frontity: Pin the react-async-helmet dependency"

fec9830

This reverts commit 3e31d8e.

Delete changeset

356a13a

Merge branch 'dev' into open-browser-tab

08a4852

luisherranz requested review from DAreRodz and removed request for DAreRodz April 15, 2020 07:04

luisherranz requested changes Apr 16, 2020

View reviewed changes

luisherranz reviewed Apr 17, 2020

View reviewed changes

packages/core/src/scripts/utils/open-browser.ts Outdated Show resolved Hide resolved

Merge branch 'dev' into open-browser-tab

b7bb663

michalczaplinski added 3 commits April 24, 2020 09:46

Merge branch 'dev' into open-browser-tab

7c12589

core: Remove unnecessary comment

e22a92e

Merge branch 'open-browser-tab' of github.com:frontity/frontity into …

dde5730

…open-browser-tab

michalczaplinski requested a review from luisherranz April 28, 2020 16:25

Merge branch 'dev' into open-browser-tab

35966b4

luisherranz requested changes Jun 1, 2020

View reviewed changes

packages/core/package.json Outdated Show resolved Hide resolved

.changeset/rare-mayflies-clap.md Outdated Show resolved Hide resolved

michalczaplinski added 4 commits June 1, 2020 11:02

Merge branch 'dev' of github.com:frontity/frontity into open-browser-tab

353d9a7

core: Update the changeset

cfebfa6

Move copyfiles to devDepenedencies

a8e9b53

Merge branch 'open-browser-tab' of github.com:frontity/frontity into …

16c663e

…open-browser-tab

michalczaplinski requested a review from luisherranz June 1, 2020 16:08

luisherranz approved these changes Jun 1, 2020

View reviewed changes

Merge branch 'dev' into open-browser-tab

2df3446

michalczaplinski merged commit 099c200 into dev Jun 2, 2020

michalczaplinski deleted the open-browser-tab branch June 2, 2020 17:36

github-actions bot mentioned this pull request Jun 3, 2020

Version Packages #457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-use an existing browser tab with with the same URL when launching `frontity dev` #374

Re-use an existing browser tab with with the same URL when launching `frontity dev` #374

michalczaplinski commented Apr 14, 2020 •

edited

changeset-bot bot commented Apr 14, 2020 •

edited

michalczaplinski commented Apr 14, 2020

luisherranz commented Apr 14, 2020

luisherranz commented Apr 14, 2020

luisherranz commented Apr 14, 2020

michalczaplinski commented Apr 14, 2020 •

edited

luisherranz commented Apr 15, 2020

luisherranz commented Apr 15, 2020

michalczaplinski commented Apr 15, 2020 via email •

edited

luisherranz commented Apr 16, 2020 •

edited

luisherranz left a comment

luisherranz Apr 16, 2020

michalczaplinski Apr 16, 2020

luisherranz Apr 17, 2020

michalczaplinski Apr 23, 2020

michalczaplinski commented Apr 16, 2020

michalczaplinski commented Apr 16, 2020

luisherranz commented Apr 17, 2020 •

edited

luisherranz commented Apr 17, 2020

michalczaplinski commented Apr 23, 2020

michalczaplinski commented Apr 23, 2020 •

edited

codecov bot commented May 29, 2020 •

edited

luisherranz commented May 29, 2020

luisherranz commented May 29, 2020

DAreRodz commented May 29, 2020

DAreRodz commented May 29, 2020 •

edited

luisherranz commented May 30, 2020

michalczaplinski commented May 30, 2020

luisherranz left a comment

Re-use an existing browser tab with with the same URL when launching frontity dev #374

Re-use an existing browser tab with with the same URL when launching frontity dev #374

Conversation

michalczaplinski commented Apr 14, 2020 • edited

changeset-bot bot commented Apr 14, 2020 • edited

🦋 Changeset is good to go

michalczaplinski commented Apr 14, 2020

luisherranz commented Apr 14, 2020

luisherranz commented Apr 14, 2020

luisherranz commented Apr 14, 2020

michalczaplinski commented Apr 14, 2020 • edited

luisherranz commented Apr 15, 2020

luisherranz commented Apr 15, 2020

michalczaplinski commented Apr 15, 2020 via email • edited

luisherranz commented Apr 16, 2020 • edited

luisherranz left a comment

Choose a reason for hiding this comment

luisherranz Apr 16, 2020

Choose a reason for hiding this comment

michalczaplinski Apr 16, 2020

Choose a reason for hiding this comment

luisherranz Apr 17, 2020

Choose a reason for hiding this comment

michalczaplinski Apr 23, 2020

Choose a reason for hiding this comment

michalczaplinski commented Apr 16, 2020

michalczaplinski commented Apr 16, 2020

luisherranz commented Apr 17, 2020 • edited

luisherranz commented Apr 17, 2020

michalczaplinski commented Apr 23, 2020

michalczaplinski commented Apr 23, 2020 • edited

codecov bot commented May 29, 2020 • edited

Codecov Report

luisherranz commented May 29, 2020

luisherranz commented May 29, 2020

DAreRodz commented May 29, 2020

DAreRodz commented May 29, 2020 • edited

luisherranz commented May 30, 2020

michalczaplinski commented May 30, 2020

luisherranz left a comment

Choose a reason for hiding this comment

Re-use an existing browser tab with with the same URL when launching `frontity dev` #374

Re-use an existing browser tab with with the same URL when launching `frontity dev` #374

michalczaplinski commented Apr 14, 2020 •

edited

changeset-bot bot commented Apr 14, 2020 •

edited

michalczaplinski commented Apr 14, 2020 •

edited

michalczaplinski commented Apr 15, 2020 via email •

edited

luisherranz commented Apr 16, 2020 •

edited

luisherranz commented Apr 17, 2020 •

edited

michalczaplinski commented Apr 23, 2020 •

edited

codecov bot commented May 29, 2020 •

edited

DAreRodz commented May 29, 2020 •

edited