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

Lazy module export #395

Closed

Conversation

CaptainN
Copy link
Contributor

@CaptainN CaptainN commented Sep 2, 2019

Make module export lazy as per #394

@aldeed
Copy link
Collaborator

aldeed commented Nov 13, 2019

I guess this would be worth it, though I don't know why Meteor does it this way. It would be better if each project could just choose to depend on it lazily without requiring a breaking change to the package like this.

@CaptainN
Copy link
Contributor Author

I agree. Meteor needs more of a peer dependency definition. This is probably a breaking change though - it would require some use cases do an intentional import. Maybe that's too much? IDK.

@aldeed
Copy link
Collaborator

aldeed commented Nov 13, 2019

I am transferring this to @Meteor-Community-Packages so I will let others help decide whether this is worth a major version release.

@harryadel harryadel mentioned this pull request Dec 2, 2019
@StorytellerCZ
Copy link
Member

I'm in agreement for major version update with this. Preferably with other changes updates like #250 so that there is a bit more goodies in the update.

@harryadel
Copy link
Member

Thank you @StorytellerCZ. I was kinda skeptical whether to do a major or minor update, as for including #250 it seems that needs more work so I'm unsure about it.

@StorytellerCZ
Copy link
Member

@harryadelb Yes, but that is out of scope for this PR. It is just matter of including things and timing the release. Anyhow @CaptainN could you please update your PR to fix the conflicts and do a major version bump. Thanks!

@CaptainN
Copy link
Contributor Author

CaptainN commented Feb 6, 2020

I actually think this would be too difficult for most users. We should probably not merge this.

@copleykj
Copy link
Member

copleykj commented Feb 6, 2020

@CaptainN why would this be difficult?

@CaptainN
Copy link
Contributor Author

CaptainN commented Feb 6, 2020

Users could have to intentionally import the package before creating their collections. It would not auto attach to the collections the way it does now.

@copleykj
Copy link
Member

copleykj commented Feb 6, 2020

What about either exporting Collection after it's modified so that it makes sense as to why you are importing something, or a combination of that and making use of the technique outlined by this article by @jankapunkt: https://dev.to/jankapunkt/lightweight-meteor-packages-with-conditional-dynamic-imports-1d51 so that it's optional?

@CaptainN
Copy link
Contributor Author

CaptainN commented Feb 6, 2020

I like the idea of exporting the modified collection (collection2) instead of modifying the original the way it does now. That's a better pattern in general, IMHO. It would allow for better code splitting and all that too.

But that's a HUGE change, and we'd need some wide discussion to catch any possible consequences that we aren't seeing. I generally support this idea though.

@CaptainN CaptainN closed this May 30, 2020
@StorytellerCZ
Copy link
Member

@CaptainN I'm all for it. Maybe start a topic on the forums?

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.

None yet

5 participants