FIX: Uploads cannot be mapped due to the cook-text's element attr being null (PR #10136)

GitHub

@eviltrout The cook-text component will always fail to resolve upload URLs because the tagName is always “”

const CookText = Component.extend({
  tagName: "",
  cooked: null,

[...]

I suspect this started happening after this commit:

https://github.com/discourse/discourse/commit/f0620e7118a76a1faea0ca15ac554818f8bb1bcf

@romanrizzi can you explain a little more? In particular I’m not sure why a missing <div> would cause uploads not to resolve?

I just noticed that I linked the wrong commit, sorry. I was talking about this one:

https://github.com/discourse/discourse/commit/293467a37ae33c5fd8fd40bbfc05bffa578b90ed

To resolve the upload’s URL, we pass this.element to the module:

next(() =>
        window
          .requireModule("pretty-text/upload-short-url")
          .resolveAllShortUrls(ajax, this.siteSettings, this.element)
      );

this.element is always null because we explicitly set tagName to be "" by default. It wasn’t a problem before the commit I linked because we used to search the whole document if there was no element.

If we keep this default, the cook-text component will always fail to resolve uploads unless a tagName gets passed to it. IMO, this could lead to unexpected behavior. We should set a different default for this component or fallback to searching the whole document as we used to.

Maybe we should remove tagName: "" from the cook-text component? Now that I look at my PR (https://github.com/discourse/discourse/pull/10097) it makes no sense for it to be tagless by default because it won’t work… :grimacing:

If the assumption is we need a tag we should add it back.

Removing the tagName: "" also does the trick. It also solves the issue of manually having to set it every time the component is called. It seems better than searching the whole document. What do you think, @eviltrout?

Yes that’s what I was suggesting, we add the div back by removing tagName.

Oh, I thought you were talking about searching the whole document by default. I’ll do that then!

:+1: (as long as there are no CSS issues :stuck_out_tongue:)