Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Initial implementation of postMessages #469

Merged
merged 22 commits into from
Mar 2, 2020
Merged

Initial implementation of postMessages #469

merged 22 commits into from
Mar 2, 2020

Conversation

ole1986
Copy link
Contributor

@ole1986 ole1986 commented Feb 25, 2020

DonJayamanne
DonJayamanne requested changes Feb 25, 2020
View reviewed changes
Copy link
Owner

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great work, few minor suggestions

browser/src/actions/results.ts Outdated Show resolved Hide resolved
browser/src/actions/results.ts Outdated Show resolved Hide resolved
browser/src/index.tsx Outdated Show resolved Hide resolved
DonJayamanne
DonJayamanne requested changes Feb 25, 2020
View reviewed changes
src/server/apiController.ts Outdated Show resolved Hide resolved
browser/src/actions/results.ts Outdated Show resolved Hide resolved
@ole1986 ole1986 force-pushed the replace-webserver-with-postmessage branch from 30557b6 to 2d08e03 Compare February 26, 2020 10:59
@ole1986 ole1986 mentioned this pull request Feb 26, 2020
Closed
@ole1986
Copy link
Contributor Author

ole1986 commented Feb 27, 2020

I have noticed using postMessages the Uri object shipped with CommittedFile using getCommit is translated from

host

{
"$mid":1,
"fsPath":"/home/user/repo/readme.txt",
"path":"/home/user/repo/readme.txt",
"scheme":"file"
}

into

browser

{
"$mid":1,
"authority":"ssh-remote+<hash>",
"path":"/home/user/repo/readme.txt",
"scheme":"vscode-remote"
}

when using it remotely which causes an error while comparing the file against workspace or previous version in the next step due to the missing fsPath

@ole1986 ole1986 changed the title Initial start of postMessage implementation Initial implementation of postMessages Feb 28, 2020
@ole1986 ole1986 linked an issue Feb 28, 2020 that may be closed by this pull request
Remove webserver #451
Closed
DonJayamanne
DonJayamanne requested changes Feb 28, 2020
View reviewed changes
.prettierrc.js Show resolved Hide resolved
@DonJayamanne
Copy link
Owner

DonJayamanne commented Feb 28, 2020 •
edited
Loading

Thanks for the PR, however in the future please could we try to limit changes to one thing. E.g. if looking at this PR next time to figure out changes that may have broken something, it's very difficult to review done other unrelated changes as well.

@ole1986
Copy link
Contributor Author

ole1986 commented Feb 28, 2020

I fully agree. I'll try my best to divide them properly next time

ole1986 added 16 commits February 29, 2020 08:59
Initial start of postMessage implementation
6e2148c
No need for localResourceRoots as the extensionPath is already included
cd5b536
Recovered authorFilter search
0f5c845
Fixed branches dropdown not updating selection
fc9d9b5
Implemented MessageBus using Promise with unique requestId
71327e5
fixed paging
a56b8bf
Fixed fsPath in Uri is unavailable during postMessage
27cbb12
replaced FsUri with native Uri
26add7b
Fixed #472
174d638
Update package-lock.json
10b7226
Lint vs gulp (#471)
8ef01c4 
* removing gulp while running tslint --fix while committing
* use object instead of any for IPostMessage
* replaced tslint ans tsfmt with eslint and prettier
* added some exceptional rules in eslintrc
* eslint --fix incl. prettier on src/
* Resolved unit test issues by properly using tabs as expected
* slightly corrected launch.json
* Use git extension api to fetch refs and remove RefParser
* removed old codecoverage from .travis.yml
* set the webpack maxAssetSize and maxEntrypointSize to 700kb
* no sourcemap for production
* use nodejs v10 in .travis
recovered detached head in branch dropdown #167
9b7b213
use node-fetch to grab github avatars
f92b050
fetch avatars only once when opening
380cd91
Fixed file status images not shown
506899d
fix ts error using any type in githubUsers
83c9ca1
@ole1986 ole1986 force-pushed the replace-webserver-with-postmessage branch from 29ade06 to 83c9ca1 Compare February 29, 2020 08:18
Update dependencies while fixing related issues (#473)
0088fab 
* Updated packages incl. typescript, react, redux, etc..
* Fixes all related issues due to the package updates
* Webpack improvements (#474)
@ole1986
Copy link
Contributor Author

ole1986 commented Feb 29, 2020 •
edited
Loading

Due to the change of webpack __dirname is broken in src/nodes/nodeIcons.ts

Luckily this seem to be the only left place where __dirname is used

@ole1986 ole1986 requested a review from DonJayamanne March 1, 2020 17:29
@DonJayamanne
Copy link
Owner

I trust the changes are ok. Will test tonight

@DonJayamanne
Copy link
Owner

Everything looked good, except for some style change that caused icons to not line up anymore..
Hews the new UI
Screen Shot 2020-03-02 at 01 44 15
Hews the old UI
Screen Shot 2020-03-02 at 01 45 08

DonJayamanne
DonJayamanne approved these changes Mar 2, 2020
View reviewed changes
Copy link
Owner

@DonJayamanne DonJayamanne left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the misaligned icons, all good.
We can fix that separately (before we ship) if u want.

@ole1986 ole1986 merged commit 9930a03 into master Mar 2, 2020
@ole1986 ole1986 deleted the replace-webserver-with-postmessage branch March 6, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DonJayamanne DonJayamanne DonJayamanne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove webserver
2 participants
@ole1986 @DonJayamanne