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

Fix: support for using require.js in pure XML #1559

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sebastien
Copy link

require.js does not work with pure-XML documents right now. This patch fixes it.

<?xml version="1.0" encoding="UTF-8"?>
<Application xmlns:html="http://www.w3.org/1999/xhtml">
    <html:script type="text/javascript" src="http://requirejs.org/docs/release/2.2.0/minified/require.js" />
        <html:script type="text/javascript">require(["underscore"]);</html:script>
</Application>

vs

<?xml version="1.0" encoding="UTF-8"?>
<Application xmlns:html="http://www.w3.org/1999/xhtml">
    <html:script type="text/javascript" src="https://raw.githubusercontent.com/sebastien/requirejs/master/require.js" />
        <html:script type="text/javascript">require(["underscore"]);</html:script>
</Application>

@jrburke
Copy link
Member

jrburke commented Jul 11, 2016

I am a bit hesitant to do too many changes to support pure XML documents, I just do not want to guarantee full support since I am not confident there are enough tests for that environment. What about the following instead, to avoid too many changes in the file:

  1. If using in an XML document, set the xhtml: true in a requirejs.config() call, so that would avoid the first change in this pull request.

  2. Allow for head to fall back to document.rootNode.firstChild, but do it as an || document.rootNode.firstChild when first defining head, and put a comment above that line to indicate the || document.rootNode.firstChild part is for XML documents.

How does that sound?

@sebastien
Copy link
Author

I definitely agree with 2). For 1) I think requirejs should not fail by default with XML documents -- I would personally consider this a bug to be fixed. Also, using xhtml:true is not semantically correct as there might be XML documents that do not use the xhtml namespace. Would that work for you?

@jrburke
Copy link
Member

jrburke commented Jul 12, 2016

The proposed fix of using !document.body for this XML case results in using document.createElementNS('http://www.w3.org/1999/xhtml', 'html:script'), which uses the xhtml namespace. It means effectively buying into that xhtml namespace, so asking XML document authors to set xhtml: true seems like a clearer intention than not seeing a document.body.

The other solution if there were very specific needs of an XML document is for it override requirejs.createNode to do custom node creation before starting module loading.

Using requirejs in XML documents has not been a popular request, given how long the project has existed and the code has existed in its current form for so long. So I do not want a fix to get too involved for such a long tail use.

@sebastien
Copy link
Author

So just to be clear you do not want to fix this issue at all, although the
changes are minor and do not affect other features? Again, I am happy to
update the PR to comply to both 1) and 2) if this is what is required for
it to be merged. Thanks.

Le lun. 11 juil. 2016 23:34, James Burke notifications@github.com a
écrit :

The proposed fix of using !document.body for this XML case results in
using document.createElementNS('http://www.w3.org/1999/xhtml',
'html:script'), which uses the xhtml namespace. It means effectively
buying into that xhtml namespace, so asking XML document authors to set
xhtml: true seems like a clearer intention than not seeing a document.body.

The other solution if there were very specific needs of an XML document is
for it override requirejs.createNode to do custom node creation before
starting module loading.

Using requirejs in XML documents has not been a popular request, given how
long the project has existed and the code has existed in its current form
for so long. So I do not want a fix to get too involved for such a long
tail use.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1559 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAA5BhlGvJfMrkpFuJgiyDvmHQo2VHMNks5qUwsggaJpZM4JIGoj
.

@jrburke
Copy link
Member

jrburke commented Jul 12, 2016

If the pull request is updated to match the 1) and 2) items above, then I am open to merging. I'll be traveling with bad internet for the next few days, so I my next response on this issue will be delayed.

@sebastien
Copy link
Author

I pushed 82a3a7c too early but 0014b9c works properly. Thanks for the feedback!

@jrburke
Copy link
Member

jrburke commented Jul 24, 2016

document.firstElementChild seems to have spotty browser support. Looks like document.rootNode has even less coverage, so good to switch away from that. Do you know if document.documentElement is available in XML documents?

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.

None yet

4 participants