Remember and Recall window layout/position state#3622
Remember and Recall window layout/position state#3622dbkr merged 9 commits intoelement-hq:developfrom t3chguy:t3chguy/electron-persist-window-layout
Conversation
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Improve dependency management for Electron main process deps Dependencies in /electron/package.json will be installed through a script in /package.json and will be bundled via electron-builder Does not affect standard webapp whatsoever Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
| "win": { | ||
| "target": "squirrel" | ||
| }, | ||
| "directories": { |
There was a problem hiding this comment.
hm, how come directories is moved up here?
There was a problem hiding this comment.
C:\Users\t3chg\WebStormProjects\riot-web>node_modules\.bin\build
⚠️ "directories" in the root is deprecated, please specify in the "build"
| "build": "node scripts/babelcheck.js && npm run build:res && npm run build:bundle", | ||
| "build:dev": "node scripts/babelcheck.js && npm run build:res && npm run build:bundle:dev", | ||
| "dist": "scripts/package.sh", | ||
| "postinstall": "cd electron && npm i", |
There was a problem hiding this comment.
this looks weird - we surely don't want to try to install all the electron crap every time someone runs npm i on riot-web?
There was a problem hiding this comment.
Currently npm i is only useful because it triggers prepublish, in a later version of npm this functionality will no longer work as prepublish will only be hit on npm publish. I guess the better alternative to installing the electron deps here is to define scripts for electron-builder which run npm i on the electron deps first.
|
looks plausible other than two questions - over to you. thanks! |
|
sounds like a plan.
|
…uild so no point making it take longer than it has to for the devs only testing webapp build:electron - hook npm run install:electron install:electron - run npm i for the electron deps electron - start the app locally for testing add to README Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
|
Back to you :) |
| ``` | ||
| npm install electron | ||
| npm run install:electron | ||
| node_modules/.bin/electron . |
There was a problem hiding this comment.
this feels fiddly and likely for folks to screw up. can't we replace all 3 lines with just npm run electron?
There was a problem hiding this comment.
I'm not sure why electron isn't a devDependency? That'd get rid of line #1
the last two could be replaced with npm run electron which has already been defined
There was a problem hiding this comment.
I feel wrong putting npm i electron into an npm script, just feels really redundant, but if the majority agree then thats obviously the way to go
There was a problem hiding this comment.
Sounds like the correct way of doing this is with the electron-builder install-app-deps in postinstall (https://github.com/electron-userland/electron-builder/wiki/Two-package.json-Structure) (assuming the 2 package.json structure is what you were going for here)
There was a problem hiding this comment.
@dbkr Matthew had a complaint when I did had linked installing these deps to postinstall: #3622 (comment)
|
@dbkr this looks okay to me, but i think i'd like a 2nd opinion as it's more your domain |
|
@t3chguy there's one new comment here. |
dbkr
left a comment
There was a problem hiding this comment.
I was expecting this to be using the electron builder 2 package.json format, but you haven't specified the app directory (and it's not the default 'app' so it doesn't look like this is what's happening, which is surprising. Is there a reason for this?
| ``` | ||
| npm install electron | ||
| npm run install:electron | ||
| node_modules/.bin/electron . |
There was a problem hiding this comment.
Sounds like the correct way of doing this is with the electron-builder install-app-deps in postinstall (https://github.com/electron-userland/electron-builder/wiki/Two-package.json-Structure) (assuming the 2 package.json structure is what you were going for here)
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
dbkr
left a comment
There was a problem hiding this comment.
OK, I think this looks good - let's give it a go.
for #2959
Signed-off-by: Michael Telatynski 7t3chguy@gmail.com