Skip to content
This repository was archived by the owner on Sep 20, 2023. It is now read-only.

Refactor using import-cost module #3

Merged
merged 1 commit into from
Aug 12, 2017

Conversation

gregorym
Copy link

@gregorym gregorym commented Aug 8, 2017

Use import-cost to calculate import sizes. I removed all the unnecessary stuff and did a bit of function renaming.

@gregorym
Copy link
Author

gregorym commented Aug 8, 2017

Takes care of this issue #2

@gregorym gregorym force-pushed the use-import-cost-module branch from ec70e59 to 8845b72 Compare August 8, 2017 21:01
@@ -7,5 +7,5 @@
opacity: 0.5;
margin-left: 10px;
position: relative;
top: -(@component-line-height - (@component-padding/2));
top: -@component-line-height;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you have to leave the padding as it is. Since it will disturb the position of the size from the top by 2px.

Copy link
Author

Choose a reason for hiding this comment

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

it looks pretty wells centered for me.
screen shot 2017-08-10 at 10 38 35 am

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm! Probably because I've changed the font! (but not sure how that can effect)

Copy link
Author

Choose a reason for hiding this comment

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

I also use a custom font. I just switched to the default font and it looks properly aligned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, in custom font, it breaks a bit bad :P (but its fine) people hardly change it ;)

Copy link
Collaborator

@reznord reznord left a comment

Choose a reason for hiding this comment

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

LGTM!

@reznord
Copy link
Collaborator

reznord commented Aug 10, 2017

@siddharthkp this changes looks good to me. Waiting for your review & merge

@siddharthkp siddharthkp merged commit da61387 into siddharthkp:master Aug 12, 2017
siddharthkp added a commit that referenced this pull request Aug 12, 2017
@siddharthkp
Copy link
Owner

Whoops, this completely stopped working for me. Reverted it for now: 30654bc

Let me dig deeper into what happened.

Copy link
Collaborator

@reznord reznord left a comment

Choose a reason for hiding this comment

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

There are a few things:

  1. loader is not working (not sure why)
  2. This works only when the package is available in node_modules folder.

@gregorym
Copy link
Author

How can I repro this issue?

@siddharthkp
Copy link
Owner

I didn't get time to debug it properly after that 😢

Let me try to get some help with this

@siddharthkp
Copy link
Owner

This is the error that I was getting.

Failed to require the main module of 'atom-import-cost' because it requires one or more incompatible native modules (binding, binding, binding, binding, binding, binding).
Run `apm rebuild` in the package directory and restart Atom to resolve.

I've published it again as atom-import-cost-beta, can you try if this works for you?

@gregorym
Copy link
Author

Ok, i will take a look.

@gregorym
Copy link
Author

import-cost has a native dependency that needs to be rebuilt by Atom.. 😢 it works after that.
I'll try to see if there is a work-around.

@gregorym
Copy link
Author

One of the dependency of import-cost depends on fsevents which causes the extension to not work after it is installed.

The work around is to rebuild the failing dependency by clicking on the red bug icon at the bottom right of Atom. it is not a great experience but that's the best we can do for now...

Examples:

@siddharthkp
Copy link
Owner

That doesn't sound good 😞

I've been playing with the idea of keeping the installs and bundling on a remote server that uses the same package and talk to it via an API.
We have add an optimisation on top of this which caches the size of package@version on the client.

Cons: Need internet access to get the size of a package@version you are using for the first time.

@reznord
Copy link
Collaborator

reznord commented Aug 22, 2017

I was just going through this: https://github.com/electron/electron/blob/master/docs/tutorial/using-native-node-modules.md

Will dig into it more and drop here if I come across something useful.

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

Successfully merging this pull request may close these issues.

3 participants