lloeki / matterfront

Mattermost frontend app for OS X, Windows and Linux
MIT License
152 stars 27 forks source link

Crashes on startup since PR #67 #73

Closed lloeki closed 8 years ago

lloeki commented 8 years ago

without a state.json:

Uncaught Exception:
TypeError: Error processing argument -1.
    at TypeError (native)
    at Object.defineProperty.set (/Users/lloeki/Workspace/projects/matterfront/node_modules/electron-prebuilt/dist/Electron.app/Contents/Resources/atom.asar/browser/api/lib/web-contents.js:98:24)
    at EventEmitter.<anonymous> (/Users/lloeki/Workspace/projects/matterfront/src/teams.js:8:23)
    at emitOne (events.js:77:13)
    at EventEmitter.emit (events.js:169:7)
    at EventEmitter.<anonymous> (/Users/lloeki/Workspace/projects/matterfront/node_modules/electron-prebuilt/dist/Electron.app/Contents/Resources/atom.asar/browser/api/lib/web-contents.js:101:27)
    at emitTwo (events.js:87:13)
    at EventEmitter.emit (events.js:172:7)

with a state.json:

Uncaught TypeError: Cannot read property 'substr' of undefined

This looks like what @H3Chief got there and apparently the fixes don't fix it. Cleaning node_modules has no effect.

lloeki commented 8 years ago

Hah, my bad for the second one. team.name unset, because you now have to set the name key in state.json.

lloeki commented 8 years ago

We should:

geekytime commented 8 years ago

validate the state structure better, and present a friendly error message about the state being corrupted

I was thinking about this last night, and my plan is to just assume a logical default name from the url. I didn't want to do that at first, due to the possibility of collisions, but it's probably a little more user-friendly than an error message. But yeah, in the case that the app state is unrecoverable, some kind of alert that the state is corrupted probably still makes sense.

Hopefully we'll get these managed in code, so they're less likely to end up broken.

Another thing we need to watch for in this is if users run multiple Matterfront processes, saving the state like this isn't going to work very well...

lloeki commented 8 years ago

Thought the same. Ideas on the spot:

Sure we can work around that for name but in the future we may have unrecoverable things, especially on major changes of state format between versions.

if users run multiple Matterfront processes, saving the state like this isn't going to work very well...

Indeed, the most obvious case being a contributor running both a stable build and its dev mode app.

LongLiveCHIEF commented 8 years ago

when I worked on this the other day, I managed to reproduce an interesting issue. I would go in and add a name attribute to the team in the state.json, but then when I ran npm run start-watch, it would actually reset the state.json file to whatever it was the last time matterfront ran.

I found that anytime start-watch was run, it overwrote the state.json file with whatever the state would have been the last time matterfront ran successfully.

When I modified the state.json file, then ran npm start instead of (or prior to) running start-watch), it would persist the changes correctly when all-windows-closed event fired. After one successful persist with the new team information, then no more incorrect overwrites would happen on start-watch.

I also found that the npm run build command would exhibit the same behavior as the start-watch command, and would overwrite state.json with whatever state was present whenever the last successful run of matterfront quit.

This meant that if you made any changes to state.json while matterfront was closed, and then ran either npm run start-watch or npm run build, the new modifications made to state.json (and likely config.json also) would be cleared.

Dennis-Smurf commented 8 years ago

in settings.js there is no defaults for teams.
After adding:

{
  "teams": [{
    "name": "team1",
    "url": "http://some.server.com/team1"
  },{
    "name": "team2",
    "url": "http://some.server.com/team2"
  }]
}

to the defaults in settings.js it worked. So you can deploy with predefined team(s).

LongLiveCHIEF commented 8 years ago

@Dennis-Smurf those settings should be entered into your ~/.matterfront/state.json, as mentioned in the updates in the README.md. This issue only occurs for those who don't follow the setup instructions, or who have followed them, but performed a very specific set of odd steps that would also cause the same behavior.

You def do not want to add these to the settings.js file if you have any intention of running a build or distributing in any way. Besides that, we are preparing to merge PR #75 which fixes this for those folks who used a released version of Matterfront, and are updating with the master (unstable) branch without following the new instructions.

RangerRick commented 8 years ago

FYI, I got this error when attempting to run an updated build from master after previously running a 1.1.x matterfront.

LongLiveCHIEF commented 8 years ago

@RangerRick that means you are missing the teams block of your state.json file.

@geekytime @3-john-4 I'm thinking of un-releasing v1.x of this until we get the UI for team additions online... I get the feeling that people aren't reading update docs, and this isn't very forgiving right now if you don't.