Skip to content

Improve extension size and performance #52

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

Merged
merged 2 commits into from
May 21, 2022

Conversation

jasonwilliams
Copy link
Contributor

@jasonwilliams jasonwilliams commented Nov 23, 2021

Before:
58.4 MB (61,314,657 bytes)
201ms startup time

After:
3.43 MB (3,607,464 bytes)
32ms startup time

  • Adds a bundler to produce a smaller extension
  • Only activates when within JS/TS file instead of on startup
  • Bumps the JS version for less boilerplate code generated
  • bumps version and minimum VSCode version

Copy link
Owner

@ipatalas ipatalas left a comment

Choose a reason for hiding this comment

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

That's pretty cool! Thanks for contributing. I'll have a look on how this works over the weekend (hopefully) and then I can merge that.

@ipatalas ipatalas linked an issue Nov 26, 2021 that may be closed by this pull request
@ipatalas
Copy link
Owner

Looks like upgrading VSCode engine to 1.60 broke the tests.
Moreover I cannot even run npm test because it fails on node build.js step.
The error is:

λ node build.js
 > error: Failed to write to output file: open <root>\vscode-postfix-ts\out\extension.js: The system cannot find the file specified.

 > error: Failed to write to output file: open <root>\vscode-postfix-ts\out\extension.js.map: The system cannot find the file specified.

Error: Build failed with 2 errors:
error: Failed to write to output file: open <root>\vscode-postfix-ts\out\extension.js: The system cannot find the file specified.
error: Failed to write to output file: open <root>\vscode-postfix-ts\out\extension.js.map: The system cannot find the file specified.
    at failureErrorWithLog (<root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:1493:15)
    at <root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:1151:28
    at runOnEndCallbacks (<root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:1069:65)
    at buildResponseToResult (<root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:1149:7)
    at <root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:1258:14
    at <root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:629:9
    at handleIncomingPacket (<root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:726:9)
    at Socket.readFromStdout (<root>\vscode-postfix-ts\node_modules\esbuild\lib\main.js:596:7)
    at Socket.emit (events.js:315:20)
    at addChunk (internal/streams/readable.js:309:12) {
  errors: [
    {
      detail: undefined,
      location: null,
      notes: [],
      pluginName: '',
      text: 'Failed to write to output file: open <root>\\vscode-postfix-ts\\out\\extension.js: The system cannot find the file specified.'
    },
    {
      detail: undefined,
      location: null,
      notes: [],
      pluginName: '',
      text: 'Failed to write to output file: open <root>\\vscode-postfix-ts\\out\\extension.js.map: The system cannot find the file specified.'
    }
  ],
  warnings: []
}

Well, apparently there is no out\extension.js because this script was supposed to generate it. I have cleaned out beforehand.
I'm not familiar with esbuild, can you help me out? For the very same reason I cannot even run the extension as the build fails with the same error.

@jasonwilliams
Copy link
Contributor Author

jasonwilliams commented Nov 26, 2021

That’s very strange, it was building fine for me. Sadly I’m not at my machine at the moment but I will take a look when I get some time. I should be able to look on Monday

@ipatalas
Copy link
Owner

ipatalas commented Nov 27, 2021

That’s very strange, it was building fine for me. Sadly I’m not at my machine at the moment but I will take a look when I get some time. I should be able to look on Monday

Ok, I'll try to figure it out on my own in the meantime. Take care.

Update: seems like it builds just fine on WSL so it must be a Windows issue - I guess you were running this on Linux?

Still the tests are not running locally but for the very same reason so at least it's easy to reproduce.
I'll try to investigate as those tests are pretty important and are keeping me sure nothing breaks.

@jasonwilliams
Copy link
Contributor Author

I’m sure I was on windows 11 when building (non-WSL)

what version of Node are you using?

@ipatalas
Copy link
Owner

I’m sure I was on windows 11 when building (non-WSL)

what version of Node are you using?

I was on node v14.16, then upgraded to v16 but still the same.
I got it, it's something strange on my end with the path. I cloned a fresh repo again and still the same. Then I cloned again in root directory and it works there 🤷🏻
Surprisingly there are no special characters in the path, if there were any I would give this a shot much earlier.

@ipatalas
Copy link
Owner

ipatalas commented Nov 27, 2021

Ok, I know why the tests are not running - it requires some adjustments which I have done on my end but to my surprise most of them failed. I ran the extension then and figured I can't see any postfix templates showing and then I remembered why I didn't bundle this extension earlier. See this:

export const loadBuiltinTemplates = () => {
const templates: IPostfixTemplate[] = []
const files = glob.sync('../templates/*.js', { cwd: __dirname })
files.forEach(path => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const builder: () => IPostfixTemplate | IPostfixTemplate[] = require(path).build

They are being loaded file based which does not make any sense after bundling :(
I see two solutions here, either it's relatively easy to exclude a folder from being bundled or rework the part which loads all the templates so that it does not depend on files.
I'll try to do the latter as this should be easy as well and is more reliable solution.

@ipatalas
Copy link
Owner

Ok, figured out how to bring back the templates to work again. This part was fairly easy. Still the problem with tests remains.
I managed to make it at least run: https://github.com/ipatalas/vscode-postfix-ts/runs/4342714606?check_suite_focus=true
Still 99 failing and not sure why :/ Good thing is that when I run this inside WSL on my machine I get exactly the same result - same tests failing and succeeding. Need some time to make this work again. Very unlikely to happen this weekend but hope to get back to this soon.

@ipatalas
Copy link
Owner

I know it's been a while but I remembered about that today and had some time to dig in. I've got the tests green: https://github.com/ipatalas/vscode-postfix-ts/runs/6538582789?check_suite_focus=true

Initially I thought it was vscode engine bump which caused it to fail but apparently it was the bundler. I haven't found any proper solution for that yet so for now I have a workaround for tests - just don't use bundled version for tests ;)
That means I can merge it.

@ipatalas ipatalas merged commit d702640 into ipatalas:develop May 21, 2022
@ipatalas ipatalas mentioned this pull request May 21, 2022
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.

[Request] - Packaging up dist with a bundler
2 participants