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

Only check versions for dev/local builds #6137

Closed
wants to merge 1 commit into from
Closed

Only check versions for dev/local builds #6137

wants to merge 1 commit into from

Conversation

maraisr
Copy link
Contributor

@maraisr maraisr commented Mar 17, 2019

Might be a tad controversial, but with this PR I aim to only enable the version checking things on local/dev builds. IMO production, deployed versions shouldn't really echo their outdated.

Or am I completely wrong, and the intent here is to tell them to upgrade wherever they are?

@github-actions
Copy link
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","doc-dependencies:update","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS against 6fe27c5

@ndelangen
Copy link
Member

Hey @maraisr thanks for this PR.

I think completely removing all functionality for the version module isn't too great.

The version notification is annoying, but the about page should still show the current and next version IMHO.

Also the module and it's api's could one day be used by addons, so I think not initializing it could lead to bugs later. Which would probably be a pain to trace, since it would only manifest in the production version.

I discussed with @shilman and we came up with a version that just hides the notification.

Do you agree this is better solution?

Again, thanks for your input and time 🙇

@maraisr
Copy link
Contributor Author

maraisr commented Mar 18, 2019

@ndelangen - oh yeah completely agree! Still learning my way around the project to have known about the other use cases 😅.

Will update this PR to only remove the toast in a PRO build.

And also; once again thank you for taking the time to even look at this pr! 😸

@maraisr
Copy link
Contributor Author

maraisr commented Mar 18, 2019

Sorry; AFAIK the latest next branch contains a mode !== 'production' that wraps that update notification, rendering this PR defunct. Someone else beat me to it 😎

Thanks again for your time 💯

Fixed with #6143

@maraisr maraisr closed this Mar 18, 2019
@maraisr maraisr deleted the no-upgrade-on-prod branch March 18, 2019 21:48
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.

2 participants