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

Filter undefined and null results from dataset object #4

Merged

Conversation

richardkazuomiller
Copy link
Contributor

uhtml-ssr currently throws an error when one of the values in dataset is undefined

/Users/richard.miller/srv/uhtml-ssr/node_modules/html-escaper/cjs/index.js:46
const escape = es => replace.call(es, ca, pe);
                             ^

TypeError: String.prototype.replace called on null or undefined
    at replace (<anonymous>)
    at escape (/Users/richard.miller/srv/uhtml-ssr/node_modules/html-escaper/cjs/index.js:46:30)
    at Object.data (/Users/richard.miller/srv/uhtml-ssr/cjs/utils.js:157:36)
    at Array.map (<anonymous>)
    at updates.push.result (/Users/richard.miller/srv/uhtml-ssr/cjs/utils.js:80:42)
    at Array.updates.<computed> (/Users/richard.miller/srv/uhtml-ssr/cjs/utils.js:140:37)
    at Array.update (/Users/richard.miller/srv/uhtml-ssr/cjs/index.js:41:17)
    at Array.map (<anonymous>)
    at fn (/Users/richard.miller/srv/uhtml-ssr/cjs/index.js:14:16)
    at Object.<anonymous> (/Users/richard.miller/srv/uhtml-ssr/test/index.js:98:22)

In uhtml, the undefined dataset property is removed, so this PR changes the behavior of uhtml-ssr's .dataset to do the same.

@WebReflection
Copy link
Owner

I've always been skeptical about these kind of normalization, for the following reason:

  • why would you pass undefined or null around in the first place?
  • this MR does not fix the described issue, it just fixes it in a specific . use case. Why not fixing it where it should, which is the replace.call(...) utility?

@richardkazuomiller
Copy link
Contributor Author

richardkazuomiller commented Jul 27, 2021

The reason I added it here was because it's closer to what uhtml does when rendering .dataset, but please correct me if I'm wrong about that.

https://github.com/WebReflection/uhandlers/blob/de8140df1b57464693b8066ec9f458c3a49a17b2/esm/index.js#L51

why would you pass undefined or null around in the first place?

In my specific use case, I have an object with an optional property. The structure is like this:

type Data = {
  required: string;
  optional?: string;
  // ... and some other stuff
}

My template is like this:

const data: Data = {required: 'something'}; // for example

const {required, optional} = data;
const dataset = {required, optional};

html`<div .dataset=${dataset}></div>`

This works in uhtml, but not in uhtml-ssr. My workaround for uhtml-ssr is to do this:

const data: Data = {required: 'something'}; // for example

const {required, optional} = data;
const dataset = {
  required,
  ...(optional ? {optional} : {})
};

html`<div .dataset=${dataset}></div>` // renders <div data-required="something"></div>

I think it would also be arguable that the output should be <div data-required="something" data-optional="undefined"></div>, since calling element.dataset.optional = undefined in the browser will set the data-optional attribute to the string "undefined", but in any case I think uhtml and uhtml-ssr should have consistent behavior, regardless of whether passing undefined/null is good or bad.

After taking a second look, false isn't filtered in uhtml so I will make it just filter out null and undefined 🙏

@richardkazuomiller richardkazuomiller changed the title Filter undefined, null, and false results from dataset object Filter undefined and null results from dataset object Jul 27, 2021
@WebReflection WebReflection merged commit 0432a7f into WebReflection:main Jul 27, 2021
@WebReflection
Copy link
Owner

Fair enough ... it's up and running, as patch. Thank you 👍

@richardkazuomiller
Copy link
Contributor Author

Thank you! 😸

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

2 participants