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

add app.isPackaged #12656

Merged
merged 9 commits into from May 8, 2018

Conversation

Projects
None yet
10 participants
@codebytere
Member

codebytere commented Apr 18, 2018

Resolves #7714.

First pass at adding an isPackaged() method to app in order to distinguish between development and production.

Todo:

  • specs
  • docs

/cc @zeke

@codebytere codebytere requested a review from electron/reviewers as a code owner Apr 18, 2018

zeke added some commits Apr 18, 2018

@codebytere codebytere requested a review from electron/docs as a code owner Apr 18, 2018

@zeke

This comment has been minimized.

Member

zeke commented Apr 18, 2018

For testing the app.isPackaged() === true scenario, we were thinking of writing a small app that does process.stdout.write(String(app.isPackaged()), packaging it with electron-packager, running it, and asserting the output. But perhaps there's a better way. Any ideas?

@@ -39,6 +41,10 @@ Object.assign(app, {
}
})
app.isPackaged = () => {
return ['app', 'app.asar'].some(p => fs.existsSync(path.join(process.resourcesPath, p)))

This comment has been minimized.

@poiru

poiru Apr 18, 2018

Member

I think this could be done without a sync fs call by just comparing against __dirname.

This comment has been minimized.

@zeke

zeke Apr 18, 2018

Member

Good idea. What would the relative path be?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 22, 2018

Member
['app', 'app.asar'].some(p => __dirname.startsWith(`${path.join(process.resourcesPath, p)}${path.sep}`))

Untested code but should do the trick without an fs call

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 22, 2018

Member

Actually thinking about it __dirname in this context will be electron.asar so that doesn't really help, I would like to find a way to do this without an fs call though

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 22, 2018

Member

Here's a fun one

app.isPackaged = () => {
  const execFile = path.basename(process.execPath)
  if (process.platform === 'win32') {
    return execFile === 'electron.exe'
  }
  return execFile === 'electron'
}

This comment has been minimized.

@codebytere

codebytere Apr 23, 2018

Member

The above seems like a best case to me

@zeke zeke changed the title from add isPackaged method to app to add app.isPackaged() Apr 19, 2018

@sindresorhus

This comment has been minimized.

Contributor

sindresorhus commented Apr 23, 2018

I think this would be better as a non-method property. It's a static check, so no need to execute it each time:

app.isPackaged = (() => {
  const execFile = path.basename(process.execPath)
  if (process.platform === 'win32') {
    return execFile === 'electron.exe'
  }
  return execFile === 'electron'
})();
app.isPackaged = () => {
const execFile = path.basename(process.execPath)
if (process.platform === 'win32') {
return execFile === 'electron.exe'

This comment has been minimized.

@MarshallOfSound

This comment has been minimized.

@codebytere

codebytere Apr 23, 2018

Member

lol whoops

if (process.platform === 'win32') {
return execFile === 'electron.exe'
}
return execFile === 'electron'

This comment has been minimized.

@MarshallOfSound

codebytere and others added some commits Apr 23, 2018

@zeke zeke changed the title from add app.isPackaged() to add app.isPackaged Apr 23, 2018

@zeke

This comment has been minimized.

Member

zeke commented Apr 23, 2018

Updated docs and tests to account for change from a function to a property. Still interested in ideas on how to test the app.isPackaged === true scenario.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 23, 2018

@zeke Testing the packaged scenario is not easy because the tests run against the debug build of Electron which is pretty big, copying it around to run tests would be pretty expensive 🤔 Maybe copying default_app.asar to app.asar and spawning process.execPath with a flag that outputs app.isPackaged 🤔

if (process.platform === 'win32') {
return execFile !== 'electron.exe'
}
return execFile !== 'electron'

This comment has been minimized.

@sethlu

sethlu Apr 24, 2018

Member

I think the base name for Electron on macOS is Electron (capitalized)?

@codebytere

This comment has been minimized.

Member

codebytere commented May 8, 2018

I've updated based on latest changes, but i'm not sure we should try to test the packaged scenario given that we would then have to rely on electron-packager which would introduce a cyclic dependency.

If we test the positive scenario locally to success (which we have), i'm comfortable just testing the false scenario in specs and leaving the true scenario out.

@ckerr

ckerr approved these changes May 8, 2018

LGTM

@codebytere codebytere merged commit daf75dd into master May 8, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@codebytere codebytere deleted the add-app-packaged branch May 8, 2018

@zeke

This comment has been minimized.

Member

zeke commented May 8, 2018

Thanks for making this happen @codebytere 🌟

@Mikaeru69

This comment has been minimized.

Mikaeru69 commented Jun 21, 2018

Until now, to distinguish between development and production , I've been using
const isPackaged = !process.defaultApp;
from the main process.
I wonder whether the planned app.isPackaged will return a different result...

process.defaultApp
A Boolean. When app is started by being passed as parameter to the default app, this property is true in the main process, otherwise it is undefined.

@zeke

This comment has been minimized.

Member

zeke commented Jun 21, 2018

Quoting from #7714

Electron has a property called process.defaultApp which is set to true when running Electron with a target directory, e.g. electron .. There are at least two problems with this:

  1. Only works in the main process. Rendered processes can't access this property by default.
  2. The name defaultApp is not intuitive, and the docs give no indication of this property's practical use.
@Mikaeru69

This comment has been minimized.

Mikaeru69 commented Jun 21, 2018

  1. Well, in order to access these properties from a renderer process, one still needs to make use of remote...

const isDefault = require('electron').remote.process.defaultApp || false;

const isPackaged = require('electron').remote.app.isPackaged;

  1. Absolutely right!
@schontz

This comment has been minimized.

schontz commented Sep 25, 2018

What version of electron is this available in?

@schontz

This comment has been minimized.

schontz commented Sep 25, 2018

Nvm. Found it on the blog post, but it hasn't made its way to the docs yet.

@shiftkey

This comment has been minimized.

@schontz

This comment has been minimized.

schontz commented Sep 25, 2018

Weird, it doesn't show up in search.

@zeke

This comment has been minimized.

Member

zeke commented Sep 25, 2018

My bad, probably. @schontz can you file an issue on https://github.com/electron/algolia-indices please?

@schontz

This comment has been minimized.

schontz commented Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment