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.

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 webpack bundling for faster startup #717

Merged
merged 3 commits into from
Dec 5, 2018
Merged

Add webpack bundling for faster startup #717

merged 3 commits into from
Dec 5, 2018

Conversation

thejewdude
Copy link

@thejewdude thejewdude commented Nov 29, 2018

Short description of what this resolves:

This PR improves extension startup time by bundling with webpack. On my local environment startup is down to 400 - 500 ms in the "Running Extensions" view.
I would love to be able to compare Javascript profiles of before and after but I don't know how to profile extension startup time in vscode.

Changes proposed in this pull request:

Fixes: #656

How Has This Been Tested?

I was able to build the extension using the npm scripts including compile, watch, and vscode:prepublish. I tested the changes on my environment and was able to download my settings stored in github gist.

My environment: windows 10; vscode 1.29.1 (user setup)

Screenshots (if appropriate):

Checklist:

  • I have read the contribution guidelines.
  • My change requires a change to the documentation and Github Wiki.
  • I have updated the documentation and Wiki accordingly.

@thejewdude
Copy link
Author

Tests seem to be broken; I'm getting this error
Error: Cannot find module 'C:\Users\theje\source\repos\code-settings-sync\node_modules\bin\mocha'

@shanalikhan
Copy link
Owner

I would love to be able to compare Javascript profiles of before and after but I don't know how to profile extension startup time in vscode.

Can you compare current version before and after your PR and post the image.

"Startup Up Performance"
image

and Running Extensions

image

@shanalikhan shanalikhan added this to the v3.2.3 milestone Nov 29, 2018
@shanalikhan shanalikhan changed the base branch from master to v3.2.3 November 29, 2018 09:39
@thejewdude
Copy link
Author

Sure thing, will get to this after work

@thejewdude
Copy link
Author

thejewdude commented Nov 30, 2018

Before:

image

image

After:

image

@shanalikhan
Copy link
Owner

Can you fix the tests also?

@thejewdude
Copy link
Author

I was able to get the tests running again but a bunch of tests are failing now. 1 failure is because of how windows handles newlines (CRLF vs LF); and 1 failure because of the difference between windows directory seperators.
Were all the passing up until now?

@thejewdude
Copy link
Author

thejewdude commented Dec 1, 2018

I actually checked out to your current master and the test (I ran npm run test) script seems to be broken. It looks like this part of the test script is causing the problem: node ./node_modules/bin/mocha. I checked the node_modules folder and the bin folder doesn't exist, at least not on my machine.

The error:

image

@thejewdude
Copy link
Author

So it seems that the reason why the tests were failing on machine is because of line ending shenanigans and git. After turning off the git config core.autocrlf the tests pass. It might be worth adding an editorconfig file to configure LF line endings as the default for the repo.

Side note: It looks like the sync pragma feature will only work with a setting file that has LF line endings since the regexp that searches for pragmas uses the LF line ending. This is not really an issue since vscode creates the settings files with LF line endings by default.

@shanalikhan is there anything else I should to do in order to get this merged?

@shanalikhan
Copy link
Owner

alright. That's perfect.

I will merge it in few days.

@shanalikhan shanalikhan merged commit 0e9cc40 into shanalikhan:v3.2.3 Dec 5, 2018
@shanalikhan
Copy link
Owner

shanalikhan commented Dec 11, 2018

Its not working at my side.
Can you create a package of extension by vsce package by pulling the branch v3.2.3

and then install it via code --install-extension
The Advance Setting menu appearing as:

image

@thejewdude
Copy link
Author

Yes i'll check this out now

@thejewdude
Copy link
Author

thejewdude commented Dec 11, 2018

The webpack dependencies are missing in package.json. It might have gotten lost when my branch was merged in, since other changes were made to the same part of package.json.
Should I rebase and open a new PR?

@shanalikhan
Copy link
Owner

The webpack dependencies are missing in package.json

Yes, ts-loader was found missing, i build after filling all dependencies in my local and got into the problem i shown in previous comment.
Since you are working, yes send PR at once after by creating vsce package and testing by installing on your local machine ( instead of testing by building)

@thejewdude
Copy link
Author

Ok I can do it later today

@thejewdude
Copy link
Author

Sorry for the delay. I wasn't able to package the extension because I was running into npm errors . Now that I sorted that out I ran into the same issue you posted. I will investigate tonight.

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

Successfully merging this pull request may close these issues.

2 participants