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

Rename load to loadComponent #19

Closed
wants to merge 1 commit into from
Closed

Conversation

CaptainN
Copy link
Contributor

Fixes a meteor module name compatibility problem, and disambiguates from the other function named load.

For some reason, meteor's build system will not export a function named "load". It should probably have been called "loadComponent" from the start anyway.

…ty problem, and disambiguates from the other function named load
@kaisermann
Copy link
Owner

I'm not completely sure about this one because it would be a breaking-change. Is there a specific reason why meteor won't allow a exported function named load?

I agree that the name loadComponent is more straight-forward, but what else can a component loader load 👀?

@kaisermann kaisermann added the question Further information is requested label Nov 21, 2019
@kaisermann kaisermann self-assigned this Nov 21, 2019
@CaptainN
Copy link
Contributor Author

CaptainN commented Nov 21, 2019

I maybe can load any module, but it's really for loading Svelte Components, so I think that label makes sense. I don't know why Meteor can't export the "load" method - it's probably a bug. If we want to retain backward compatibility, maybe we can export both, and treat "load" as deprecated? I'll check with Meteor about why the load method is not being exported.

(I also remembered I have to update the readme if this goes through.)

@CaptainN
Copy link
Contributor Author

I added an issue in the Meteor issue tracker. Let's see what they say.

@benjamn
Copy link

benjamn commented Nov 22, 2019

Yes, let's figure out what the issue is on Meteor's side before we ask other projects to rename their methods. Since the entry point is Loadable.svelte, my guess is that the issue is happening in the Meteor compiler plugin for .svelte files. Whatever it is, I'm pretty sure it's not any fault of the svelte-loadable package.

@CaptainN
Copy link
Contributor Author

I agree we should fix it in meteor or svelte-meteor.

I half asked them to rename the function because I named it load originally, and feel it was a mistake. O.o

@klaussner
Copy link

This looks like a Svelte bug (not related to Meteor or meteor-svelte). I've opened an issue here: sveltejs/svelte#3983.

@CaptainN
Copy link
Contributor Author

CaptainN commented Nov 24, 2019

Hmm, I must not have noticed due to a problem I was having updating passed svelte 3.6.10 (which seems to have been caused by a problem in meteor core). When I figured that out, the was the very next issue I had to fix.

It makes me wonder whether exporting load from the module has ever worked, practically speaking (at least in non meteor-svelte projects, which are locked to an even older version of svelte in the current non-beta release). Probably has only been broken for 2 weeks on the cutting edge version, so not a big deal.

In favor of renaming:

  • IMO, it should probably have been loadComponent originally anyway (leaving it as load was my mistake - we even already have a copy of that defined internally).
  • It works around this likely temporary bug in the short term.
  • It probably hasn't ever worked with contemporary versions of svelte, since the feature is not that old.

In favor of not renaming it:

  • We released it as load and so there may be some projects which rely on that - better not to change it.
  • Maybe the generic load is preferred?

Suggested workaround - rename it in code and readme to loadComponent (or loadModule which I don't prefer), and also export a load for backward compatibility.

@klaussner
Copy link

It makes me wonder whether exporting load from the module has ever worked, practically speaking (at least in non meteor-svelte projects, which are locked to an even older version of svelte in the current non-beta release).

I just tried out a few versions using the Svelte REPL. The bug was introduced in version 3.13.0 (released two weeks ago).

@CaptainN
Copy link
Contributor Author

That's good to know. If the argument for changing it is not strong enough, please close the PR. Thanks!

@kaisermann kaisermann closed this Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants