fix(app): display app version again #14844
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we switched to vite, we had to switch all the stuff we'd been injecting at pack time via webpack environment/define plugins to vite's `define` config functionality. The biggest thing we specify that way is the app version, which is used across the stack for display and for logic. In the commit that switched to vite, we added that injection for the app-shell vite configs but did not add it for the app vite configs. That meant that at runtime, the version value was undefined, which breaks robot update notifications and causes the app version in the general settings tab to not display (it also makes the logo wrong on internal releases but that's a bit less important). The fix is to inject the version into the app build again. This is made a little more complicated because if you're doing stuff to the app vite config, it has to work in both the vite devserver and the vite offline packaging environments, and the vite devserver doesn't allow commonjs, and the git-version script that gives us the version is commonjs. For the purposes of vite's devserver, "doesn't work with cjs" actually just means "doesn't support require()", so you can use a hybrid syntax that uses import-statements but still module.export instead of export statements. Unfortunately, the git-version script is also used in the electron-builder config for the app-shell and the app-shell-odd, and the electron-builder config is run via node, and to import an ESM from a node CJS script - which electron-builder.config.js is - you need to change your import syntax to dynamic import and you need to make the import target explicitly (to node) an ESM, aka change its extension, and you need to use full ESM syntax including exports. So that means that - git-version.js becomes git-version.mjs and uses full ESM syntax - that means that everywhere it's imported we need to import it by full path with extension instead of module name - also we need to import it dynamically in the electron config - oh and we need to actually add the define configs so we get the version in the app And then finally we show the version again. Closes EXEC-385
koji
reviewed
Apr 9, 2024
app/vite.config.ts
Outdated
| @@ -1,62 +1,70 @@ | |||
| import path from 'path' | |||
| import { defineConfig } from 'vite' | |||
| import { UserConfig, defineConfig } from 'vite' | |||
There was a problem hiding this comment.
Suggested change
| import { UserConfig, defineConfig } from 'vite' | |
| import {defineConfig } from 'vite' | |
| import type { UserConfig } from 'vite' |
koji
approved these changes
Apr 9, 2024
6 tasks
sfoster1
added a commit
that referenced
this pull request
Apr 16, 2024
Fixes various ongoing issues building versions into the system. This is a follow-on to #14844 (61b1371) - vite `define` config the way we do it does not hang the defined values off of props of `global` explicitly (or maybe us injecting `'globalThis'` into `global` breaks it) so use them as true globals, altering the way they're accessed and the way they're declared in the typings. - i guess you don't actually have to do type imports in the top level typings? removing the type import of the ipc bridge in the app-shell and app-shell-odd global.d.ts fixed that issue. don't know why - the ESM import for the script that updates the releases.json was wrong which breaks some update stuff ## Testing This is a bit annoying to test. You _must_ test this on a compiled app package. You _cannot_ test this on a dev build. On a compiled app package, - [x] the version should display in the settings tab of the app - [x] you shouldn't have warnings about `include` on undefined in your app logs - [ ] you should get robot update prompts when you use an internal-release build and connect to a robot running 1.3 or previous; you should get robot update prompts when you use a release build and connect to a robot running 7.2.1 or previous (note: couldn't test this in time but the rest of it works) - [x] the help menu should have a bugs url that works (the "report an issue" button; it should pop a browser tab) - [x] the help menu should say "View Opentrons App Logs" or "View Opentrons OT-3 App Logs" as the variant demands - [x] it should NOT say "View App Logs". that means the package name wasn't properly interpolated. I haven't dev-tested the second part because on my home setup making a full compiled app package is broken for some reason, and you can't actually run the node side in dev This once more, Closes EXEC-385 Closes RQA-2579
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
When we switched to vite, we had to switch all the stuff we'd been injecting at pack time via webpack environment/define plugins to vite's `define` config functionality. The biggest thing we specify that way is the app version, which is used across the stack for display and for logic. In the commit that switched to vite, we added that injection for the app-shell vite configs but did not add it for the app vite configs. That meant that at runtime, the version value was undefined, which breaks robot update notifications and causes the app version in the general settings tab to not display (it also makes the logo wrong on internal releases but that's a bit less important). The fix is to inject the version into the app build again. This is made a little more complicated because if you're doing stuff to the app vite config, it has to work in both the vite devserver and the vite offline packaging environments, and the vite devserver doesn't allow commonjs, and the git-version script that gives us the version is commonjs. For the purposes of vite's devserver, "doesn't work with cjs" actually just means "doesn't support require()", so you can use a hybrid syntax that uses import-statements but still module.export instead of export statements. Unfortunately, the git-version script is also used in the electron-builder config for the app-shell and the app-shell-odd, and the electron-builder config is run via node, and to import an ESM from a node CJS script - which electron-builder.config.js is - you need to change your import syntax to dynamic import and you need to make the import target explicitly (to node) an ESM, aka change its extension, and you need to use full ESM syntax including exports. This also goes for the create-release script. So that means that - git-version.js becomes git-version.mjs and uses full ESM syntax - that means that everywhere it's imported we need to import it by full path with extension instead of module name - also we need to import it dynamically in the electron config - oh and we need to actually add the define configs so we get the version in the app And then finally we show the version again. Also, remove some old webpack.config.js files that aren't used anymore. Closes EXEC-385
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
Fixes various ongoing issues building versions into the system. This is a follow-on to #14844 (61b1371) - vite `define` config the way we do it does not hang the defined values off of props of `global` explicitly (or maybe us injecting `'globalThis'` into `global` breaks it) so use them as true globals, altering the way they're accessed and the way they're declared in the typings. - i guess you don't actually have to do type imports in the top level typings? removing the type import of the ipc bridge in the app-shell and app-shell-odd global.d.ts fixed that issue. don't know why - the ESM import for the script that updates the releases.json was wrong which breaks some update stuff ## Testing This is a bit annoying to test. You _must_ test this on a compiled app package. You _cannot_ test this on a dev build. On a compiled app package, - [x] the version should display in the settings tab of the app - [x] you shouldn't have warnings about `include` on undefined in your app logs - [ ] you should get robot update prompts when you use an internal-release build and connect to a robot running 1.3 or previous; you should get robot update prompts when you use a release build and connect to a robot running 7.2.1 or previous (note: couldn't test this in time but the rest of it works) - [x] the help menu should have a bugs url that works (the "report an issue" button; it should pop a browser tab) - [x] the help menu should say "View Opentrons App Logs" or "View Opentrons OT-3 App Logs" as the variant demands - [x] it should NOT say "View App Logs". that means the package name wasn't properly interpolated. I haven't dev-tested the second part because on my home setup making a full compiled app package is broken for some reason, and you can't actually run the node side in dev This once more, Closes EXEC-385 Closes RQA-2579
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we switched to vite, we had to switch all the stuff we'd been injecting at pack time via webpack environment/define plugins to vite's
defineconfig functionality. The biggest thing we specify that way is the app version, which is used across the stack for display and for logic.In the commit that switched to vite, we added that injection for the app-shell vite configs but did not add it for the app vite configs. That meant that at runtime, the version value was undefined, which breaks robot update notifications and causes the app version in the general settings tab to not display (it also makes the logo wrong on internal releases but that's a bit less important).
The fix is to inject the version into the app build again. This is made a little more complicated because if you're doing stuff to the app vite config, it has to work in both the vite devserver and the vite offline packaging environments, and the vite devserver doesn't allow commonjs, and the git-version script that gives us the version is commonjs. For the purposes of vite's devserver, "doesn't work with cjs" actually just means "doesn't support require()", so you can use a hybrid syntax that uses import-statements but still module.export instead of export statements.
Unfortunately, the git-version script is also used in the electron-builder config for the app-shell and the app-shell-odd, and the electron-builder config is run via node, and to import an ESM from a node CJS script - which electron-builder.config.js is - you need to change your import syntax to dynamic import and you need to make the import target explicitly (to node) an ESM, aka change its extension, and you need to use full ESM syntax including exports.
So that means that
And then finally we show the version again.
Closes EXEC-385