Skip to content
This repository has been archived by the owner on Jul 12, 2018. It is now read-only.

avoid multiple export per each module version #15

Merged
merged 3 commits into from
Jan 23, 2016
Merged

Conversation

WebReflection
Copy link
Owner

Currently, we are repeatedly exporting over and over same module, even if it has been already required elsewhere. This PR ensure every namespace is exported once, improving performance, avoiding duplicated/undesired behaviors (e.g. GNode.importNS('Gtk') !== GNode.importNS('Gtk'))

@@ -1,3 +1,6 @@
// used to avoid exporting same module
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put comments on same line and use capitalization. Put this variable declaration below the imports please, near where it's used.

Copy link
Owner Author

Choose a reason for hiding this comment

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

hoisting and var don't work like that and usually var declarations are all on top. Anyway, I don't mind this change.

@magcius
Copy link
Collaborator

magcius commented Jan 11, 2016

I don't know if we should be allowing loading multiple versions of the library at the same time. Doesn't seem like a good idea to me.

@WebReflection
Copy link
Owner Author

I don't know if we should be allowing loading multiple versions of the library at the same time. Doesn't seem like a good idea to me.

I've read this only now. Is there any technical problem or it's just an opinion?

I agree but if two modules would like to use Webkit and Webkit2 and they don't know each other I think that should be allowed so it shouldn't be our responsibility to prevent that.

What do you think?

@magcius
Copy link
Collaborator

magcius commented Jan 23, 2016

GTK+3 and GTK+2 certainly aren't parallel loadable. WebKit and WebKit2 have different GIR names, so that's different.

@WebReflection
Copy link
Owner Author

So I dont' understand how are you planning to solve. The problem doesn't seem to be related to this PR, it's the importNS API that accepts a name and a version, I've just made that compatible with the cache.

Which one could be a problem? And again, if two modules don't know each other but both load from node-gtk what do you think should happen?

@WebReflection
Copy link
Owner Author

where am I not using four space exactly?

@WebReflection
Copy link
Owner Author

@magcius do you realize you are commenting randomly things that have been fixed a week ago ... right?

@magcius
Copy link
Collaborator

magcius commented Jan 23, 2016

oh, oops, somehow GitHub showed me an old diff.

@WebReflection
Copy link
Owner Author

so, 12 days ago, after you commented, I've pushed all changes you have asked.
a6bfcd5

Now you raised a problem that has nothing to do with this PR, it's rather a general importNS API problem.

Do you think we can move forward with this? Because it's blocking this one too:
#19

@magcius
Copy link
Collaborator

magcius commented Jan 23, 2016

sure, let's just merge this for now.

magcius added a commit that referenced this pull request Jan 23, 2016
avoid multiple export per each module version
@magcius magcius merged commit 4996772 into master Jan 23, 2016
@magcius magcius deleted the cached-modules branch January 23, 2016 17:50
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.

None yet

2 participants