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.

Sign up for GitHub

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

Jump to bottom

Allow disabling warning on exit in development #1176

Merged
merged 5 commits into from
May 6, 2020
Merged

Allow disabling warning on exit in development #1176

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e384837
Don’t warn on exit in development
merapi Apr 13, 2020
834f731
Narrow down to development only (we have production/test/development?)
merapi Apr 13, 2020
1701365
Cleaner solution
merapi Apr 13, 2020
0f08f44
Use env var
merapi Apr 27, 2020
3b4c007
Add info to docs
merapi May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions assets/src/edit-story/utils/usePreventWindowUnload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ function usePreventWindowUnload() {
},
[context]
);

return setPreventUnload;
}

export default usePreventWindowUnload;
const isDevelopment = process.env.NODE_ENV === 'development';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export default isDevelopment ? () => {} : usePreventWindowUnload;
2 changes: 1 addition & 1 deletion assets/src/edit-story/utils/useWhyDidYouUpdate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,6 @@ function useWhyDidYouUpdate(name, props) {
});
}

const isDevelopment = process.env.NODE_ENV !== 'production';
const isDevelopment = process.env.NODE_ENV === 'development';

export default isDevelopment ? useWhyDidYouUpdate : () => {};