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

Feature > x-unsafe-html #99

Merged
merged 10 commits into from Mar 15, 2021

Conversation

SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Mar 11, 2021

Custom directives, whoop whoop!

I kept the structure similar to the helper to keep them consistent.
When we add the second one, we might want to extract some common pieces of code into utils.

Copy link
Collaborator

@ryangjchandler ryangjchandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just me being picky!

src/unsafeHTML.js Outdated Show resolved Hide resolved
src/unsafeHTML.js Outdated Show resolved Hide resolved
SimoTod and others added 2 commits March 11, 2021 21:13
Co-authored-by: Ryan Chandler <ryangjchandler@gmail.com>
@ryangjchandler
Copy link
Collaborator

Definitely agree RE extracting into utils in the future.

Copy link
Collaborator

@HugoDF HugoDF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful

@KevinBatdorf
Copy link
Collaborator

This is pretty neat. Is it possible to get it to work like this?

<div x-data="{foo: 'loading...', bar: 'yoooo'}">
    <button @click="foo = '<p>Success</p><script>console.log(bar)</script>'">
        Test
    </button>
    <div x-unsafe-html="foo"></div>
</div>

where bar is available? I would expect that to work, and actually I can't figure out why it's not working, as it appears to be passed in with the dataContext. I even tried adding it to extraVars with no luck:

const scopeVariables = { ...extraVars, ...component.$data }
el.innerHTML = component.evaluateReturnExpression(el, expression, () => scopeVariables)

If we can't get that working I think maybe we shouldn't document using it without including a component (like your example 2), which works great and as expected ($parent works too, etc), and instead let users just discover using it a la your first example.

What do you think?

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 13, 2021

A normal javascript tag doesn't have access to the Alpine variables in general. I don't expect that to work and I don't know if it would be possible. It's like writing

<div x-data="{foo: 'bar'}">
 <script>console.log(foo)</script>
</div>

right?

I think you would use it mainly to instantiate something like maps, datepickers or any other third party script in your new fragment of html

@KevinBatdorf
Copy link
Collaborator

Yeah but I thought we were parsing that out and running the expression through Alpine. I'll look through the code more closely tomorrow.

Do you think it would be better with that context or is it just me?

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 13, 2021

I think you can do something like
foo = '<p>Success</p><script>console.log(&qout;' + bar + '&quot;)</script>' in your example and that would be a valid snippet but in general you won't write your html in line like that.

@KevinBatdorf
Copy link
Collaborator

KevinBatdorf commented Mar 13, 2021

Was just messing around with it a bit. I'm not sure how practical this is or if there are any side effects

https://codepen.io/KevinBatdorf/pen/vYybjaM

Will check back in tomorrow. Gotta go for now

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 13, 2021

I was more tinking about people using it for stuff like https://codepen.io/SimoTod/pen/XWNOPQE

That does the trick but I personally would not expect it to work in that way: Alpine variables are only available in alpine directives and components. Maybe it's just me though.

The two examples below would work in a different way, for example, but they look the same to me.

<div x-data="{ foo: 'bar' }">
  <div><script>alert(foo)</script></div>
</div>
<div x-data="{ foo: 'bar', baz: '<script>alert(foo)</script>' }">
  <div x-unsafe-html="baz"></div>
</div>

Also, the script could be an external one with the src attribute, would it have access to the Alpine variables in that case

Do you have in mind an example where we would need something like that? I'd rather put a disclaimer in the readme saying that normal script tags run in the global scope and they don't have access to the component variables unless a lot of people really need it (maybe we can ask on discord?).

@ryangjchandler
Copy link
Collaborator

A quick thought, since we're hijacking the resolveBoundAttributes, we could probably also add in a custom modifier for x-html right? Find the x-html attribute and check if the modifiers include .unsafe, then run our custom logic then.

What do you think? Feels nicer, in my opinion, since you can still use the same old x-html directive.

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 13, 2021

I prefer not to touch native directives because people will start opening bugs on the official repo but, mainly, I don't know how to add a modifier on an existing directive. 😅

@ryangjchandler
Copy link
Collaborator

I prefer not to touch native directives because people will start opening bugs on the official repo but, mainly, I don't know how to add a modifier on an existing directive. 😅

Well, I think it would work a bit like you've already done it. Inside of the attrs.forEach call, just check if it has that particular modifier and if it doesn't, return early and let Alpine handle it instead.

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 13, 2021

I'm not sure we can do it because that function processes all the directives in a loop so we can't early return. I might be wrong though. Do you have a working example?

@ryangjchandler
Copy link
Collaborator

I'm not sure we can do it because that function processes all the directives in a loop so we can't early return. I might be wrong though. Do you have a working example?

I'll put something together for you tomorrow, need to try it myself!

@KevinBatdorf
Copy link
Collaborator

Do you have in mind an example where we would need something like that?

I don't feel too strongly about it either way. We can see if the community expects it to behave a certain way and make changes later. Maybe we could make it optional with a modifier even? x-unsafe-html.scoped=""? (not suggesting we should implement that now, but just a thought).

I think ideally it would behave the same as x-html would though, but with the addition of being able to parse external script tags, that's all.

But I think this is great to merge now, then iterate later if needed.

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 14, 2021

The modifier is a great idea. It keeps it in line with the browser default behaviour and allows user to make weird things if they explicitly want to. When do we think we should release it? Midweek? Friday? Monday? I personally like Monday releases.

@KevinBatdorf
Copy link
Collaborator

Monday works! You want to make the release when you get online? I can set up the codepen demo after that here: https://codepen.io/KevinBatdorf/pen/poNYpZb

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 15, 2021

I'll release it in 2h when my daughter goes to bed. 👍

@SimoTod SimoTod merged commit 08ccee9 into alpine-collective:master Mar 15, 2021
@ryangjchandler
Copy link
Collaborator

@SimoTod Right on time!

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 15, 2021

@ryangjchandler we can rename it if needed but I was keen on releasing it. Personally I'm not sure about adding a modifier to the existing x-html because people will raise bugs on the official repo but we can vote and if the majority agrees (and we have a solid implementation that extends the current behaviour without overriding it) I'm happy to switch.

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