Allow disabling warning on exit in development #1176

merapi · 2020-04-13T19:45:27Z

Don't warn on exit in development because It is a bit annoying (especially when you use auto-reload).

+isDevelopment variable narrow down to development only (we have production/test/development?).

github-actions · 2020-04-13T19:48:48Z

Size Change: -41.7 kB (5%) ✅

Total Size: 826 kB

Filename	Size	Change
`assets/css/stories-dashboard.css`	3.04 kB	+37 B (1%)
`assets/js/edit-story.js`	397 kB	-46.2 kB (11%)	👏
`assets/js/stories-dashboard.js`	423 kB	+4.46 kB (1%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	3.04 kB	0 B

_{compressed-size-action}

assets/src/edit-story/utils/usePreventWindowUnload.js

obetomuniz · 2020-04-13T20:52:27Z

I agree with this DX improvement, it's a pain sometimes haha. But the inconsistency created between environments concerns me a bit too, once the UX step created when the user needs to leave the page with any pending stuff is important, which with this change, will not be validated always while developing. WDYT @merapi ?

merapi · 2020-04-13T21:20:20Z

I think it's okay in dev mode (we know how it works and it's more for the end-user).
Alternatively, we can base it on env var that you can set in the terminal before you run webpack.

dvoytenko · 2020-04-14T16:33:30Z

assets/src/edit-story/utils/usePreventWindowUnload.js

  return setPreventUnload;
 }

-export default usePreventWindowUnload;
+const isDevelopment = process.env.NODE_ENV === 'development';


I'm generally ok with this, but I also think that for a short while this would be useful to spot the history bugs which we had a few. Maybe we can wait for a bit before applying this change?

There should be reliable tests in place at least to ensure that this works as expected before implementing this.

I personally definitely prefer my local env to be a copy of the production even if it's a bit annoying. Perhaps there should be a separate variable for disabling this in dev environment which would be off by default but could be turned on for the devs who prefer that.

I'll also keep it on - I don't mind it.

But we can simply change these lines to:

const shouldDisablePrevent = Boolean(process.env.DISABLE_PREVENT); export default shouldDisablePrevent ? () => {} : usePreventWindowUnload;

And then you can just run DISABLE_PREVENT=1 npm run dev locally if you want to prevent it? Would that be sufficient for your usecase, @merapi?

Agree, .env/env var is the way to go here.

codecov · 2020-04-27T22:05:11Z

Codecov Report

Merging #1176 into master will increase coverage by 5.59%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##             master    #1176      +/-   ##
============================================
+ Coverage     52.23%   57.82%   +5.59%     
+ Complexity      241      215      -26     
============================================
  Files           461      528      +67     
  Lines          7710     8702     +992     
============================================
+ Hits           4027     5032    +1005     
+ Misses         3683     3670      -13

Flag	Coverage Δ	Complexity Δ
#javascript	`?`	`?`
#php	`?`	`?`

Impacted Files	Coverage Δ	Complexity Δ
assets/src/edit-story/utils/useWhyDidYouUpdate.js	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...ets/src/edit-story/utils/usePreventWindowUnload.js	`85.71% <100.00%> (+1.09%)`	`0.00 <0.00> (ø)`
assets/src/dashboard/app/index.js	`0.00% <0.00%> (-33.34%)`	`0.00% <0.00%> (ø%)`
...sets/src/dashboard/app/views/shared/pageHeading.js	`0.00% <0.00%> (-33.34%)`	`0.00% <0.00%> (ø%)`
assets/src/dashboard/app/views/shared/noResults.js	`0.00% <0.00%> (-30.00%)`	`0.00% <0.00%> (ø%)`
...sets/src/dashboard/app/views/shared/bodyWrapper.js	`0.00% <0.00%> (-25.00%)`	`0.00% <0.00%> (ø%)`
...ts/src/dashboard/app/views/shared/storyGridView.js	`0.00% <0.00%> (-25.00%)`	`0.00% <0.00%> (ø%)`
assets/src/dashboard/utils/keyboardOnlyOutline.js	`0.00% <0.00%> (-20.00%)`	`0.00% <0.00%> (ø%)`
.../src/dashboard/app/views/shared/typeaheadSearch.js	`0.00% <0.00%> (-16.67%)`	`0.00% <0.00%> (ø%)`
...toryReducer/reducers/updateElementsByResourceId.js	`88.88% <0.00%> (-11.12%)`	`0.00% <0.00%> (ø%)`
... and 221 more

obetomuniz · 2020-04-28T19:28:27Z

assets/src/edit-story/utils/usePreventWindowUnload.js

  return setPreventUnload;
 }

-export default usePreventWindowUnload;
+const shouldDisablePrevent = Boolean(process.env.DISABLE_PREVENT);


Would be good to document it somewhere. WDYT? Maybe under the docs folder creating an ENV_VARS.md file?

Added some info in 3b4c007

dvoytenko

LGMT with one nit.

dvoytenko · 2020-05-06T15:45:00Z

docs/getting-started.md

@@ -83,6 +83,10 @@ To get a production build, run:
 npm run build:js
 ```

+### Live reload
+
+We don't provide it OOTB. You can setup your own solution and `DISABLE_PREVENT=1 npm run dev` will help you with unwanted `beforeunload` alert.


IMHO, =true/=false are more natural.

It's cast to bool in the code, so it's just the docs that'd need update.

Will do that in #1476

* master: (53 commits) Inline text formatting (#1323) Bump terser-webpack-plugin from 2.3.6 to 3.0.1 (#1597) Allow disabling warning on exit in development (#1176) fixing indent that was missed by automation Deploy plugin bundle to wiki repo for easier QA (#1590) Fix initial height of preset panel (#1591) Fixes editor resizing after fullbleed (#1553) More error handling Fix file drag & drop crash for unsupported media types (#1431) Improve test Bump polished from 3.6.1 to 3.6.2 (#1577) Bump react-transition-group from 4.3.0 to 4.4.0 (#1565) Remove Delete Page shortcut tooltip (#1574) renaming filteredStories to just stories. they are not filtered. updating typeahead input and options for new designs. New colors, tighter space, different expanded view. Removing option to filter internally through typeahead since we do all of that outside of the component. some styled component cleanup Remove wp element. Bump @testing-library/dom from 7.2.2 to 7.3.0 (#1563) Remove wp element Lints in tests Add more tests ...

* master: (109 commits) Inline text formatting (#1323) Bump terser-webpack-plugin from 2.3.6 to 3.0.1 (#1597) Allow disabling warning on exit in development (#1176) fixing indent that was missed by automation Deploy plugin bundle to wiki repo for easier QA (#1590) Fix initial height of preset panel (#1591) Fixes editor resizing after fullbleed (#1553) More error handling Fix file drag & drop crash for unsupported media types (#1431) Improve test Bump polished from 3.6.1 to 3.6.2 (#1577) Bump react-transition-group from 4.3.0 to 4.4.0 (#1565) Remove Delete Page shortcut tooltip (#1574) renaming filteredStories to just stories. they are not filtered. updating typeahead input and options for new designs. New colors, tighter space, different expanded view. Removing option to filter internally through typeahead since we do all of that outside of the component. some styled component cleanup Remove wp element. Bump @testing-library/dom from 7.2.2 to 7.3.0 (#1563) Remove wp element Lints in tests Add more tests ...

merapi added 2 commits April 13, 2020 21:41

Don’t warn on exit in development

e384837

Narrow down to development only (we have production/test/development?)

834f731

merapi requested review from swissspidy and obetomuniz April 13, 2020 19:45

merapi requested review from barklund and dvoytenko as code owners April 13, 2020 19:45

merapi self-assigned this Apr 13, 2020

googlebot added the cla: yes label Apr 13, 2020

obetomuniz reviewed Apr 13, 2020

View reviewed changes

assets/src/edit-story/utils/usePreventWindowUnload.js Outdated Show resolved Hide resolved

Cleaner solution

1701365

dvoytenko suggested changes Apr 14, 2020

View reviewed changes

Use env var

0f08f44

merapi requested review from dvoytenko, obetomuniz and miina April 27, 2020 21:57

obetomuniz reviewed Apr 28, 2020

View reviewed changes

Add info to docs

3b4c007

merapi requested a review from pbakaus as a code owner May 6, 2020 11:16

dvoytenko approved these changes May 6, 2020

View reviewed changes

swissspidy changed the title ~~Don't warn on exit in development~~ Allow disabling warning on exit in development May 6, 2020

swissspidy merged commit fc01e08 into master May 6, 2020

swissspidy deleted the try/dont-warn-on-exit-in-dev branch May 6, 2020 17:32

swissspidy mentioned this pull request May 6, 2020

Slight improvements to e2e test setup #1560

Merged

swissspidy added the Type: Infrastructure Changes impacting testing infrastructure or build tooling label May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow disabling warning on exit in development #1176

Allow disabling warning on exit in development #1176

merapi commented Apr 13, 2020 •

edited

github-actions bot commented Apr 13, 2020 •

edited

obetomuniz commented Apr 13, 2020 •

edited

merapi commented Apr 13, 2020

dvoytenko Apr 14, 2020

miina Apr 23, 2020

barklund Apr 23, 2020

merapi Apr 27, 2020

codecov bot commented Apr 27, 2020 •

edited

obetomuniz Apr 28, 2020 •

edited

merapi May 6, 2020

dvoytenko left a comment

dvoytenko May 6, 2020

swissspidy May 6, 2020

Allow disabling warning on exit in development #1176

Allow disabling warning on exit in development #1176

Conversation

merapi commented Apr 13, 2020 • edited

github-actions bot commented Apr 13, 2020 • edited

obetomuniz commented Apr 13, 2020 • edited

merapi commented Apr 13, 2020

dvoytenko Apr 14, 2020

Choose a reason for hiding this comment

miina Apr 23, 2020

Choose a reason for hiding this comment

barklund Apr 23, 2020

Choose a reason for hiding this comment

merapi Apr 27, 2020

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2020 • edited

Codecov Report

obetomuniz Apr 28, 2020 • edited

Choose a reason for hiding this comment

merapi May 6, 2020

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

dvoytenko May 6, 2020

Choose a reason for hiding this comment

swissspidy May 6, 2020

Choose a reason for hiding this comment

merapi commented Apr 13, 2020 •

edited

github-actions bot commented Apr 13, 2020 •

edited

obetomuniz commented Apr 13, 2020 •

edited

codecov bot commented Apr 27, 2020 •

edited

obetomuniz Apr 28, 2020 •

edited