-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Add expose loader and expose sortable to window #85
Conversation
What does this solves? I think it's problematic for extensions to expose global methods or libraries as you can't expect the other extension to be enabled and there could be version conflicts if multiple extensions do it. But I do think it would be great if Flarum included a global library to make sortable fields as it's a recurring thing extensions needs. But then this would need to be moved to core. |
Sorry for the lack of context, this was prompted by #82 (comment). The 'expecting extensions to be enabled' issue should hopefully eventually be solved in flarum/framework#2188, but I see your point. However, if we add all that stuff to core, it can result in a bloated core? I think that we can probably give bundled extensions a bit of special treatment here. |
Not sure where we want to discuss this, but I added why I think this is a good idea & useful @ #82 (comment). We don't want to expose this in the forum, though. It's only used in the admin (and I think other exts only use sorting like this in the admin). This import call would replace |
I don't think it makes sense to merge this PR at this time. It would be best to open a dedicated issue about extensions loading common JS dependencies, deciding on a best practice, then implement that best practice in the Tags extension. If we go for conditional dependency loading, then Tags must also consider that the dependency might have already been loaded by another core or community extension. |
The conditional dependency loading wouldn't be in core extensions. It would be in third party extensions that decide to implement it. Merging this PR will not have any effect and will keep everything the same unless extensions implement the conditional lazy loading. There's no downside to merging this PR as far as I can see. |
I feel like making I'm pretty sure this exact issue played out with an early version of Mason/Masquerade or another similar extension, but I'm unable to find traces of the issue. IMO it's not offering us any benefit as Flarum developers, and might introduce headaches later on depending what community extensions decide to do with this new "feature". If we want this library to be global, I'd say we move it to core and officially announce it to third-party developers. Otherwise wait until we have a best practice to suggest to developers regarding often-used javascript libraries and implement it in Tags the same way we suggest third-party developers do. I think we need an issue to discuss this further, in particular in regard to library versions. Can this mess up if extensions need a different version of the library? I remember jQuery support was removed at some point, so extensions might still be using another version. Can there be conflicts? A PR probably isn't the right place to talk about those use cases and edge cases. |
No, they can import their own and use that instead. But it'll have to be bundled - not exported to window, as that'd replace the other one.
It benefits the user & decreases bundle size if extensions take advantage of this.
Then the library could be provided with nobody using it. It's a good chunk of size that matters. Not sure if we'd want to do this, but perhaps do something like fof/components does? It adds an extender for extensions to request the addition of fof/components to the JS file - they are only added once.
I think this was html5sortable, maybe? Or whatever we used before it, not sure. |
Can we use expose loader in core to expose expose loader with expose loader?