kaisermann / svelte-loadable

Dynamically load a svelte component
MIT License
320 stars 13 forks source link

Rename load to loadComponent #19

Closed CaptainN closed 4 years ago

CaptainN commented 4 years ago

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.

kaisermann commented 4 years ago

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 👀?

CaptainN commented 4 years ago

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 commented 4 years ago

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

benjamn commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

CaptainN commented 4 years ago

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:

In favor of not renaming it:

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 commented 4 years ago

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 commented 4 years ago

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