Skip to content

Reworks the build/bundling#2518

Merged
RMacfarlane merged 4 commits intomainfrom
eamodio/build
Feb 22, 2021
Merged

Reworks the build/bundling#2518
RMacfarlane merged 4 commits intomainfrom
eamodio/build

Conversation

@eamodio
Copy link
Copy Markdown
Contributor

@eamodio eamodio commented Feb 22, 2021

Slims the initial bundle size
Splits the webviews out of the initial bundle to optimize loading perf
Replaces TSLint with ESLint
Replaces moment with day.js to reduce size & improve perf
Updates many package dependencies
Adds esbuild option (on by default)

Splits the webviews out of the initial bundle to optimize loading perf
Replaces moment with day.js to reduce size & improve perf
Updates many package dependencies
src/api/api.d.ts Outdated
Tag
}
export { RefType } from './api1';
// export const enum RefType {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can be removed

src/api/api.d.ts Outdated
PatchDoesNotApply = 'PatchDoesNotApply'
}
export { GitErrorCodes } from './api1';
// export const enum GitErrorCodes {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also removable

return [
uri,
{
hostname: (await HostHelper.getApiHost(hostUri)).authority,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

uri.authority?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call!


private _iter: IKeyIterator;
private _root: TernarySearchTreeNode<E> | undefined;
static forPaths<E>(): TernarySearchTree<string, E> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

several unused things here - forPaths, forStrings, forConfigKeys. should we wait to introduce these utils until they're actually needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Up to you -- I just grabbed the entire thing from the vscode repo (to maybe easier to keep it "in sync")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that's a fair point, let's leave it as is

</head>
<body>
<div id="app"></div>
<script nonce="${nonce}" src="${this._webview!.asWebviewUri(uri).toString()}"></script>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this also works in the webworker case?

import { sep } from 'path';
import moment = require('moment');
import { Disposable, Event, Uri } from 'vscode';
import dayjs from 'dayjs';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice 👍

}

return new PullRequestModel(this._telemetry, this, this.remote, convertRESTPullRequestToRawPullRequest(pullRequest, this));
return new PullRequestModel(this._telemetry, this, this.remote, convertRESTPullRequestToRawPullRequest(pullRequest as any, this));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this cast to any is unneeded

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, not needed anymore

if (reviewManager) {
return pushAndCreatePR(currentIssue.manager, reviewManager, this._stateManager);
}
break;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice

repositories_url: { default: 'https://api.github.com/teams/1/repos' },
parent: { default: { test: 'test' } },
html_url: { default: 'https://api.github.com/teams/1' }
// parent: { default: { test: 'test' } },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

removable

src/extension.ts Outdated
}

export async function activate(context: vscode.ExtensionContext): Promise<GitApiImpl> {
extensionId = 'github.vscode-pull-request-github';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is actually EXTENSION_ID used below which should capture this and is changed in the build, it could just be marked as not const to be reused here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks!

Copy link
Copy Markdown
Contributor

@RMacfarlane RMacfarlane left a comment

Choose a reason for hiding this comment

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

Looks good!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants