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

Rollup - improve loading time #302

Merged
merged 25 commits into from
Jul 10, 2020
Merged

Rollup - improve loading time #302

merged 25 commits into from
Jul 10, 2020

Conversation

aminya
Copy link
Member

@aminya aminya commented May 29, 2020

  • This makes the tool-bar load. reduces the actual loading time (if activation hook was not enabled)

Note: benchmark is done on top of #304

After rollup:

Before (considering no drawGutter):
image

For registering run the following:

npm install
npm run ppublish
apm publish patch

Copy link
Contributor

@ericcornelissen ericcornelissen left a comment

Choose a reason for hiding this comment

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

A few comments on the implementation below - do you have some numbers on how much this improves the loading time?

Also, is it the intention that the contents of dist/ end up in the repo at some point? If so, could you add a .gitattributes file with the following contents:

* text=auto

# don't diff machine generated files
lib/ -diff
package-lock.json -diff

rollup.config.js Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
ppublish.sh Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/touch-bar-manager.js Show resolved Hide resolved
@aminya
Copy link
Member Author

aminya commented May 30, 2020

A few comments on the implementation below - do you have some numbers on how much this improves the loading time?

It loads ~20ms faster (100->80). If we rollup the css files that gives another 30ms (I tried this). But for that we need to include ui-variables. If you are OK with that we can do that.

This is related to my other PR #299

I asked this question in the slack group but there was no answer


Is there a way to rollup less files in Atom. Because of @import "ui-variables";, I cannot do that. But if I copy a ui-variables.less file, the rollup works perfectly and I get huge speed ups.
Is there a such thing in less? Something like:
src: url("atom://defaulttheme/ui-variables.less") format("less");

This is the issue I created: xiaofuzi/rollup-plugin-less#30


Also, is it the intention that the contents of dist/ end up in the repo at some point? If so, could you add a .gitattributes file with the following contents:

Yes, we have to include dist folder. That is the limitation of apm. 😞

@ericcornelissen
Copy link
Contributor

But for that we need to include ui-variables. If you are OK with that we can do that.

As per #299, that seems like a bad idea to me.

Yes, we have to include dist folder. That is the limitation of apm. 😞

Ow well, okay 👍

@aminya
Copy link
Member Author

aminya commented Jun 2, 2020

I added a script to rollup iconsets. That gives us another 10ms! 😃

@aminya aminya mentioned this pull request Jun 3, 2020
@suda
Copy link
Collaborator

suda commented Jun 3, 2020

Nice! I agree that copying ui-variables is not ideal and with rolling up both code and iconsets, it might be enough.

@suda
Copy link
Collaborator

suda commented Jun 3, 2020

I'd also prefer the dist files to be in repo so all this dance of monkey patching the repo for apm publish could be removed in favor of simple npm version hook like this: https://github.com/particle-iot/particle-api-js/blob/master/package.json#L23

iconsets/rollup-iconsets.js Outdated Show resolved Hide resolved
rollup.config.js Outdated Show resolved Hide resolved
@aminya
Copy link
Member Author

aminya commented Jun 3, 2020

@suda The issue with only using git add is that once someone runs npm run build, those files will be tracked by git and will show up in the changelogs and commits (although being in gitignore).

This bash script circumvents that issue, and only adds the dist files through npm run ppublish, and after that git will not track them.

@aminya
Copy link
Member Author

aminya commented Jun 7, 2020

I made an npm package for ppublish script. https://github.com/aminya/build-commit
I will publish it tomorrow and using that instead of the bash script here.

@aminya
Copy link
Member Author

aminya commented Jul 8, 2020

@suda Could you merge this?

@suda suda merged commit 70d99ef into atom-community:master Jul 10, 2020
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.

3 participants