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

Add expose loader and expose sortable to window #85

Closed
wants to merge 1 commit into from

Conversation

askvortsov1
Copy link
Sponsor Member

Can we use expose loader in core to expose expose loader with expose loader?

@clarkwinkelmann
Copy link
Member

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.

@askvortsov1
Copy link
Sponsor Member Author

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.

@dsevillamartin
Copy link
Member

dsevillamartin commented May 31, 2020

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 import sortable from 'sortablejs' in that file, not in a common file.

@clarkwinkelmann
Copy link
Member

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.

@dsevillamartin
Copy link
Member

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.

@clarkwinkelmann
Copy link
Member

I feel like making sortable available globally could encourage bad practice from extension developers expecting it to be available anytime, not realizing it's being provided by the Tags extension.

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.

@dsevillamartin
Copy link
Member

dsevillamartin commented May 31, 2020

Can this mess up if extensions need a different version of the library?

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.

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".

It benefits the user & decreases bundle size if extensions take advantage of this.

If we want this library to be global, I'd say we move it to core

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'm pretty sure this exact issue played out with an early version of Mason/Masquerade or another similar extension

I think this was html5sortable, maybe? Or whatever we used before it, not sure.

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.

3 participants