-
-
Notifications
You must be signed in to change notification settings - Fork 432
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
feat: pass web app version to snjs application #623
Conversation
@@ -81,6 +83,12 @@ class ApplicationViewCtrl extends PureViewCtrl<unknown, { | |||
await this.application.launch(); | |||
} | |||
|
|||
async setAppVersion() { | |||
const appVersion = packageJson.version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value is already available globally: https://github.com/standardnotes/web/blob/develop/webpack.config.js#L17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that, thanks! Will update it.
Internal link - https://app.asana.com/0/1200588672119490/1200383926936362 |
@@ -25,6 +25,8 @@ import { NativeExtManager } from '@/services/nativeExtManager'; | |||
import { StatusManager } from '@/services/statusManager'; | |||
import { ThemeManager } from '@/services/themeManager'; | |||
|
|||
declare const __VERSION__: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be imported from somewhere instead? Perhaps we should have a version.ts file that all other files reference if they need to access the version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice point, moved this and other globals to separate files. Actually my intention was to combine them in one file and do named exports, but only default export works in this case, so created a separate file for every global variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, found a better way, used approach from here.
…id declaring them in all places where they are used
app/assets/javascripts/version.ts
Outdated
declare const __DESKTOP__: boolean; | ||
declare const __WEB__: boolean; | ||
|
||
export const appVersion = __VERSION__; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I would make these PascalCase to indicate a global constant rather than var or function.
export const appVersion = __VERSION__; | |
export const AppVersion = __VERSION__; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, will update. Do you think we should make isDesktopPlatform
and isWebPlatform
Pascal case as well? I think based on their name it's better to keep them camel cased, but I'm open for other opinions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup same thing for those as well since they are global constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated)
…i-calls # Conflicts: # package.json # yarn.lock
No description provided.