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

Replace otto with goja #2975

Closed
htdvisser opened this issue Jul 24, 2020 · 3 comments · Fixed by #3152
Closed

Replace otto with goja #2975

htdvisser opened this issue Jul 24, 2020 · 3 comments · Fixed by #3152
Assignees
Labels
c/application server This is related to the Application Server in progress We're working on it
Milestone

Comments

@htdvisser
Copy link
Contributor

Summary

We should replace the unmaintained otto library with goja.

Refs #2670 and #2966

Why do we need this?

Because we should avoid unmaintained libraries, especially ones that we use to run arbitrary code.

@htdvisser htdvisser added the c/application server This is related to the Application Server label Jul 24, 2020
@htdvisser htdvisser added this to the Next Up milestone Jul 24, 2020
@htdvisser
Copy link
Contributor Author

And while at it, it would be nice to add a Prometheus histogram to get some insight in how long those scripts take

@johanstokking
Copy link
Member

johanstokking commented Jul 27, 2020

Let's do this in one go with #2670

@johanstokking
Copy link
Member

johanstokking commented Aug 20, 2020

I've spent a half day on this, and the conclusion so far is:

https://github.com/dop251/goja

https://github.com/robertkrimen/otto

  • unmaintained for a long time
  • new problems with Go 1.15
  • relies on a fork to protect against stack overflows when exporting nested objects with circular references
  • supports memory allocation limit

So, this neither are ideal. I would still opt to switch to goja because it is maintained and faster than otto.

I'm afraid we need to pick either to keep the builds simple for the open source distribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/application server This is related to the Application Server in progress We're working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants