-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
This approach has its own set of versioning issues - basically it's guaranteed someone will be caught out by us using a different library version from what they have specified in Looking at the implementation: So if I did Might a better approach be to throw during initialization as this should make Also we should have tests for running this style of plugin. |
Addressing the feedback:
|
src/worker/vm/vm.ts
Outdated
@@ -15,7 +16,11 @@ export async function createPluginConfigVM( | |||
pluginConfig: PluginConfig, // NB! might have team_id = 0 | |||
indexJs: string | |||
): Promise<PluginConfigVMReponse> { | |||
const transformedCode = transformCode(indexJs, server) | |||
const imports = { | |||
'node-fetch': { default: fetch }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly dumb question: by providing fetch
outright, will nobody be able to access some properties in a way that affects other VMs (e.g. redefine fetch.isRedirect
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not a dumb question. VM2 should wrap everything in proxies. Worth testing it out though. This PR won't really change anything security-wise though, so might be worth tackling after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation lgtm! :)
Thanks! Just some flaky tests being flaky here. I'll run them one more time (have to -> merged with master) and if it's just the |
…rver#284) * Allow whitelisted imports in babel * add test for fetching via `import` * Reword whitelisted → provided * also support "require" and handle defaults nodejs style * support requires * fix test Co-authored-by: Michael Matloka <[email protected]>
Changes
import anything from 'anywhere'
withconst anything = __pluginHostImports['anywhere']['anything']
Checklist