This repository was archived by the owner on Jun 26, 2020. It is now read-only.
Windows Build Doc & Package.json for Windows #54
Closed
Conversation
This file contains hidden or 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
Summary:
Currently, when we auth an account for the first time in Nylas Mail (or
we blow away the database), the app is going to request transactions
since cursor `null` from the /delta/streaming endpoint and from the local-sync
delta observable, instead of requesting transactions since cursor `0`
This is due to a subtle bug with the use of default values when
destructuring an object. Our coded did the following:
```
const {cursor = 0} = this._state
```
Which at a glance seems correct. However, this will only work as
expected if `this._state` has the following shape:
```
{cursor: undefined}
```
And unfortunately, our `this._state` looked like this when authing an
account for the first time:
```
{cursor: null}
```
Which would make `cursor === null` instead of `0`.
This is because when using default values, null is considered an
intentional argument/value, as opposed to not passing any argument/value
(which will mean that the argument is undefined).
This was a regression introduced in d60a23c and 8bc2ec5
Test Plan: manual, will add regression test in upcoming diff
Reviewers: evan, spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D4243
Summary: Previously, the logic for deciding wether to report an APIError lived inside NylasAPIRequest and depended on an array of ignorable statusCodes. However, when handling APIErrors, NylasAPIRequest would convert special error codes (for offline errors) into a statusCode of 0, which would automatically be ignored. Given that the delta streaming connection didn't convert status codes to 0, it wouldn't actually ignore the error and end up reporting it, flooding sentry. This commit makes it so that that logic lives inside APIError, and anyone who handles APIErrors can check wether to report them or not, including the delta streaming connection Test Plan: manual :( Reviewers: spang, evan, halla Reviewed By: evan Differential Revision: https://phab.nylas.com/D4235
Summary: This fixes T7995. Previously, attempting to log out from your NylasID would just automatically sign you back in with the same NylasID because the webview session was preserved. Now, instead of relaunching the windows, we restart the app to clear the webview session Test Plan: manual Reviewers: halla, evan Reviewed By: evan Maniphest Tasks: T7995 Differential Revision: https://phab.nylas.com/D4224
Summary: In preparation for removing timeout handling from the IMAPConnectionPool. Test Plan: Run locally Reviewers: spang, evan, juan Reviewed By: evan, juan Differential Revision: https://phab.nylas.com/D4245
Summary: Using `await` instead of `advanceClock()` fixes all the things! Test Plan: Ran the spec file Reviewers: evan, juan Reviewed By: juan Differential Revision: https://phab.nylas.com/D4248
On the server the env var is defined but set to false.
Summary: Found a funny send-later bug I didn't catch when testing on staging: sometimes the data we're saving in the metadata table overflows. That's because MySQL's TEXT column are at most 64k, which is easy to reach when you have a draft + clearbit information and additional stuff. To work around this, I decided to switch the database type of the metadata table to LONGTEXT. Since it can store 4Gb of text, we should be good. This diff makes those code changes. Obviously, we'll have to run migrations both on staging and prod. Test Plan: Ran a basic smoke test. Shouldn't break anything. Reviewers: juan, evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D4250
Summary: We need to download the files and then treat them as uploads. Rather than using an actual Upload object, which would require another data transfer, we create an object with all the necessary Upload-like properties and point it to the downloaded file. Addresses part of T7960 Test Plan: Manual, some specs Reviewers: evan, spang, juan Reviewed By: juan Differential Revision: https://phab.nylas.com/D4249
Summary: The original name seems like it's initiating the download, when really it's just returning the data of an already in-progress/completed download. Test Plan: Manual, specs Reviewers: evan, juan Reviewed By: juan Differential Revision: https://phab.nylas.com/D4251
Summary: We'd like to get an idea of how long the client app is having to wait when requesting a file. Waiting can cause things like inline image attachments to appear very slow to load. Test Plan: Run locally Reviewers: evan, spang, juan Reviewed By: juan Differential Revision: https://phab.nylas.com/D4255
Summary: Different clients can have different policies for retrying after timeouts. Test Plan: Run locally, run tests Reviewers: evan, spang, juan Reviewed By: juan Differential Revision: https://phab.nylas.com/D4247
Summary: See title Test Plan: manual Reviewers: gleb, mark Reviewed By: mark Differential Revision: https://phab.nylas.com/D4256
…rter Summary: I had a bit of downtime this morning so I decided to look into how to store Electron crash data. Electron, like Chromium, stores crash data in an arcane file format named minidump. There's a bunch of services you can use to store files formatted in this format, like Mozilla's Socorro or zcbenz's own mini-breakpad-server but they're all pretty hard to install. Luckily, it turns out there's a Ruby gem to process minidump files and send them to Sentry! Seeing that, I whipped up a quick sinatra service and hosted it on Heroku – you can take a look at the source code here if you're curious: https://github.com/khamidou/electron-breakpad-sentry This diff adds the client-side code we need to start sending crash reports. Test Plan: Tested manually. Reviewers: juan, evan Reviewed By: juan, evan Maniphest Tasks: T7926 Differential Revision: https://phab.nylas.com/D4259
Summary: Before this commit, it was impossible to remove inline images via the x button Test Plan: manual Reviewers: mark, halla Reviewed By: mark, halla Differential Revision: https://phab.nylas.com/D4257
Summary: The IdentityStore can trigger any number of times, but we only want to start sync if we previously didn't have an identity available Test Plan: manual Reviewers: spang, evan, halla Reviewed By: evan, halla Differential Revision: https://phab.nylas.com/D4246
Summary: We weren't doing that, so we would have accurate search index info for threads when they were, e.g., moved to another folder. Test Plan: Run locally Reviewers: spang, evan, juan Reviewed By: evan, juan Differential Revision: https://phab.nylas.com/D4260
Summary: These only really matter for n1cloud, and are being added manually in production, so no migration file necessary. I am adding them to the model definitions to prevent them getting out of sync with production for development. See T7999 for full SQL review Test Plan: ship it Reviewers: evan, khamidou, juan Reviewed By: juan Subscribers: vlad, jerm Differential Revision: https://phab.nylas.com/D4230
Summary: Should help a fair bit with our redis connection pileup. Test Plan: Tested manually. Reviewers: mark, spang, juan, evan Reviewed By: mark, spang, evan Differential Revision: https://phab.nylas.com/D3916
add early returns for auto-updater
Release 2.1.0
Fixes #38 Previously when editing or adding to a message that you were forwarding, upon sending the message all edits would be lost. This appears to fix the issue. * Change forceWrite param to use this.draft.
Removed -b alias for --benchmark option which collides with --background option.
Resolve -b option collision
Adjust first download section according to the second
Moves the unsubscribe plugin into an internal package. * init: `unsubscribe` as internal package Integrate n1-unsubscribe into nylas-mail directly See: https://github.com/colinking/n1-unsubscribe * fix broken N1 api request and fix out-of-date “N1” references with “Nylas Mail”
The current JSON file cause errors because of the file separators. I added new commands inside the file specially for Windows. Every windows command is with a prefix 'windows-' The build installation.md file also reflects that and follow the steps I took to build
package.json
Outdated
| @@ -42,18 +42,26 @@ | |||
| "underscore": "1.8.x" | |||
| }, | |||
| "scripts": { | |||
| "windows-start":"npm run client-windows", | |||
There was a problem hiding this comment.
Is client-windows correct here? Shouldn't it be windows-client?
| npm run windows-build-client | ||
| ``` | ||
| 7. If there are no errors in the previous two steps, start the app with: | ||
| ```bash |
|
@dweremeichik I made the fixes and pushed them. Regarding your question
I don't know honestly. I used Command Prompt for my build. I don't know what @seesemichaelj used. |
|
@dweremeichik I'm going to give this all a try and will get back tomorrow with my review |
|
Dependencies are not stylized in a list: |
|
Replicating your instructions cannot execute |
|
What is the error you get? @seesemichaelj |
|
It could not find the Windows SDK 8.1. I guess I didn't check that when installing the community version. |
|
You mean the VS community installation? |
|
Yes; I believe the Windows 8.1 SDK was not installed by default when installing VS Community. It was under "Windows 8.1 and ... Tools" or something like that |
|
Yup that definitely was the error; all of the isomorphic core stuff is building now |
|
we could get away from multiple build commands by using node scripts like https://shapeshed.com/writing-cross-platform-node/#scripts-in-package-json |
|
Now I'm getting the error Doing nodejs/node-gyp#972 (comment) ( That seems to get me to work for npm install. I'm not sure how much of this should be documented. Maybe a "troubleshooting" section? |
mikeseese
suggested changes
Aug 22, 2017
| @@ -35,3 +35,33 @@ If you are looking to simply install Nylas-Mail on your system (and are not look | |||
| npm start | |||
| ``` | |||
| * If an error was thrown during the build process, please make sure all of your dependencies were installed in step 2. | |||
|
|
|||
| ## Windows: | |||
| 1. Install Node.js version 6.9.x ( Can use Windows [NVM](https://github.com/coreybutler/nvm-windows)) | |||
There was a problem hiding this comment.
Remove space before "Can"
| ## Windows: | ||
| 1. Install Node.js version 6.9.x ( Can use Windows [NVM](https://github.com/coreybutler/nvm-windows)) | ||
| 2. Dependencies: | ||
| A. Visual Studio 2015 Community |
There was a problem hiding this comment.
A/B/C/D are not styled properly, see 
| 2. Dependencies: | ||
| A. Visual Studio 2015 Community | ||
| B. Python 2.7 (v3.x.x is not supported) | ||
| C. create a environment variable PYTHON pointing at :\\path\\python.exe |
There was a problem hiding this comment.
I think environment variables can be a separate step and not dependencies.
I would also specify how you can create an environment variable. One option is doing this in the command prompt with set VARIABLENAME=VARIABLEVALUE
|
This PR is no longer applicable due to the conflicts of the new merge. Please rebase to the new There are also some discrepancies previously mentioned that need to be addressed. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The current JSON file cause errors because of the file separators. I added new commands inside the file specially for Windows. Every windows command is with a prefix 'windows-'
The build installation.md file also reflects that and follow the steps I took to build
PR for Issue #18