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

Remove all tampering with the env (setfenv, getfenv) #1504

Open
5 tasks
SuperCater opened this issue May 2, 2024 · 7 comments
Open
5 tasks

Remove all tampering with the env (setfenv, getfenv) #1504

SuperCater opened this issue May 2, 2024 · 7 comments
Labels
✨ enhancement Enhancing or improving existing functionality 🎏 miscellaneous Miscellaneous content

Comments

@SuperCater
Copy link
Contributor

What part of Adonis is this related to?

Other

What are you suggesting?

Preface

Adonis has for a long timed used setfenv and getfenv for adding variables to module factory functions but these cause performance issues that we should seek to resolve.

Reasons to remove

Both setfenv, and getfenv have since been deprecated by Roblox and disable Luau optimizations, they're also just considered bad practice now. It also offers no ability of type checking.

Downsides

This will break the following things if fully removed

  • Any old plugins that relied on ENV
  • Any custom UI modules that relied on the ENV.

Solution

Just use the Vargs table passed to the module returns. Any method needed could be attached to the server/client table.

Each module will need to be updated to support not having a custom env.

  • Server Modules/core
  • Client Modules/core
  • All UI Modules
  • Service table
  • Plugins

In regards to the downside, i believe it would be justified to

Give an Adonis alert that plugins will break if they rely on the env as a heads up before adding this change (hopefully with an ETA of when it will be done)
Maybe add support in the loader to load specific versions for people who arent ready to upgrade yet? This would require mapping each Adonis version to its Roblox version i believe.

@SuperCater SuperCater added the ✨ enhancement Enhancing or improving existing functionality label May 2, 2024
@github-actions github-actions bot added the 🎏 miscellaneous Miscellaneous content label May 2, 2024
@ocelot81
Copy link
Contributor

ocelot81 commented May 2, 2024

completely defenving Adonis especially in clientside and service will be tough and it'll be basically rewriting it completely (also due to all the security and prevention ie. ACLI) - it's possible though and definitely worth it

@ccuser44
Copy link
Contributor

ccuser44 commented May 3, 2024

Maybe this should be a 2.0 thing?
Although de-fevning plugins may be a great idea

@ocelot81
Copy link
Contributor

ocelot81 commented May 3, 2024

Maybe this should be a 2.0 thing? Although de-fevning plugins may be a great idea

Server plugins no longer have their env set #1490

@ccuser44
Copy link
Contributor

ccuser44 commented May 3, 2024

Maybe this should be a 2.0 thing? Although de-fevning plugins may be a great idea

Server plugins no longer have their env set #1490

Probably broke some things but thats fine
Im worried about how this will affect dex though

@ocelot81
Copy link
Contributor

ocelot81 commented May 3, 2024

I was thinking of including dex(s) there too but they're unchanged, still with setfenv

@ccuser44
Copy link
Contributor

ccuser44 commented May 3, 2024

I was thinking of including dex(s) there too but they're unchanged, still with setfenv

One thing to note that even though the plugin doesn't manually setfenv Adonis itself still calls setfenv on all modules from Client.lua and Server.lua

@SuperCater
Copy link
Contributor Author

Maybe this should be a 2.0 thing?

It would be nice but sadly 2.0 has no active development at the moment and probably wont be done for years.

@Epix-Incorporated Epix-Incorporated deleted a comment Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement Enhancing or improving existing functionality 🎏 miscellaneous Miscellaneous content
Projects
None yet
Development

No branches or pull requests

3 participants