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 js exit behaviour #56

Open
wants to merge 2 commits into
base: ansi
Choose a base branch
from
Open

fix js exit behaviour #56

wants to merge 2 commits into from

Conversation

TanklesXL
Copy link
Owner

@TanklesXL TanklesXL commented Aug 19, 2024

This change is a preliminary bugfix in order to addess #55

@TanklesXL TanklesXL changed the title fix js exit behaviour fix js exit behaviour #55 Aug 29, 2024
@TanklesXL TanklesXL changed the title fix js exit behaviour #55 fix js exit behaviour Aug 29, 2024
@TanklesXL
Copy link
Owner Author

@ghivert does this behaviour properly un-break glint in the browser?

src/glint_ffi.mjs Outdated Show resolved Hide resolved
@ghivert
Copy link

ghivert commented Aug 29, 2024

Isn't it dangerous to just do this? Because the process will continue to execute. Are you sure it will stop all other operations? 🙂

(By the way, I did not know GitHub allows to review PR like that, even if you're not on the repo. Weird experience. I just clicked on "leave single comment", so do not take it as me trying to review properly. 😅)

@TanklesXL
Copy link
Owner Author

Isn't it dangerous to just do this? Because the process will continue to execute. Are you sure it will stop all other operations? 🙂

(By the way, I did not know GitHub allows to review PR like that, even if you're not on the repo. Weird experience. I just clicked on "leave single comment", so do not take it as me trying to review properly. 😅)

That's a good point, this won't stop the current process from executing, this is effectively the same as running glint with glint.without_exit. (maybe it's worth just setting that if in the browser and documenting that explicitly?)

As a workaround right now, there is the exported but undocumented alternative to glint.run that is glint.execute that is

@internal
pub fn execute(glint: Glint(a), args: List(String)) -> Result(Out(a), String)

i wonder if that is a decent option

@TanklesXL
Copy link
Owner Author

TanklesXL commented Aug 29, 2024

Thinking about this more, with it being an implicit glint.without_exit, glint will still behave and run normally it just wont kill execution, it just won't end the current process. I can't stop a developer from putting code after glint executes (nor do i think i'd want to)

Co-authored-by: Guillaume Hivert <[email protected]>
@ghivert
Copy link

ghivert commented Oct 15, 2024

Do you want me to properly test the change? 🙂

@TanklesXL
Copy link
Owner Author

Thanks for the offer! after thinking about it a bit more, I'm planning some upcoming changes for glint v2 that will rip all IO out of the core so I think it may not be necessary

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.

2 participants