vendor/sanitize should be in excludeShallow #750

Closed
wimleers opened this Issue Oct 5, 2012 · 7 comments

Projects

None yet

3 participants

@wimleers
Contributor
wimleers commented Oct 5, 2012

vendor/sanitize.js is currently being aggregated into the aloha.js builds. However, as soon as sanitizecontenthandler.js tries to use it, it will fail, because sanitize.js is not an AMD module. It should be excluded.

In the Drupal build, I'm doing this to fix it in my build profile:

excludeShallow: [ "vendor/sanitize" ]

I imagine you need to do the same for the default builds.

@wimleers
Contributor
wimleers commented Oct 5, 2012

For the record: this is what broke pasting into Aloha in Drupal: http://drupal.org/node/1748692.

@deliminator
Contributor

I don't understand the reasoning behind this? Why exclude it? Wouldn't it then be loaded dynamically by requirejs?

@wimleers
Contributor
wimleers commented Oct 6, 2012

That's what I thought too. But it's actually broken: it's in a closure, meaning that the Sanitize object is not in the global (window) namespace, thus making it unavailable for e.g. sanitizecontenthandler.js to use.

@deliminator
Contributor

I think I understand now, please correct me if I'm wrong: everything works during development, since everything is loaded dynamically by requirejs without being wrapped in a closure, but when you build it with the drupal build profile, where the core and each plugin are separate modules that are wrapped by a closures, the global Santize function in sanitize.js, which is in the core module, will be hidden from the contenthandler plugin module.

If we do it like I proposed in #737, then the solution is to take santize.js out from the core and make the user include it separately, since it's a vendor file. And, it only needs to be included, if the user is actually using the sanitize content handler.

@wimleers
Contributor
wimleers commented Oct 8, 2012

Your detailed explanation is completely correct. Sorry for not providing a more exhaustive example.

@evo42
Member
evo42 commented Oct 8, 2012

I talked with Haymo some time ago about sanitize.js & we will / should replace sanitize.js with https://code.google.com/p/jquery-clean/

--> in that iteration I'd like also to fix the issue we talked about that contentHandler should get a jQuery object passed instead of a string

@deliminator
Contributor

I've looked at jquery-clean, and it only works on string input as well, not on DOM nodes.

Maybe it would be a good idea to just do it ourselves. It shouldn't be difficult.

@evo42 evo42 was assigned Oct 13, 2012
@evo42 evo42 closed this Oct 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment