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

createImports converts functions to objects #333

Open
trusktr opened this issue Mar 1, 2021 · 5 comments
Open

createImports converts functions to objects #333

trusktr opened this issue Mar 1, 2021 · 5 comments
Assignees
Labels
Question ❔ Further information is requested Waiting 🛑 Something is waiting

Comments

@trusktr
Copy link
Contributor

trusktr commented Mar 1, 2021

https://github.com/jtenner/as-pect/blob/67fda969e0691869bace7444543490fb3a319c13/packages/core/src/test/TestContext.ts#L536

That line incorrectly assumes all properties are objects. If the import object has a method, this will replace it with an empty object.

I don't think there is a need to (try to) clone everything anyway.

@trusktr
Copy link
Contributor Author

trusktr commented Mar 1, 2021

To work around the problem of createImports converting functions to empty objects, I changed my as-pect imports() config option from

	imports(memory, createImports, instantiateSync, binary) {
		const imports = createImports(initASWebGLue({env: {seed: Date.now, memory}}))

		const result = instantiateSync(binary, imports)

		imports.setExports(result.exports)
		imports.shouldLogAborts(false)

		return result
	},

to

	imports(memory, createImports, instantiateSync, binary) {
		let imports = initASWebGLue({env: {seed: Date.now, memory}})

		// We need to grab these methods first, otherwise as-pect's
		// createImports function currently has an issue that will convert the
		// functions into empty objects.
		// https://github.com/jtenner/as-pect/issues/333
		const {shouldLogAborts, setExports} = imports

		imports = createImports(imports)

		const result = instantiateSync(binary, imports)

		setExports(result.exports)
		shouldLogAborts(false)

		return result
	},

The pull request in glas is here: lume/glas#122

@jtenner
Copy link
Contributor

jtenner commented Mar 1, 2021

Ah. Yeah. I understand. As-pect assumes the shape of the imports object. In fact, as-pect doesn't want to mutate your imports object. It returns a brand new one.

	imports(memory, createImports, instantiateSync, binary) {
		let imports = initASWebGLue({env: {seed: Date.now, memory}})

                // This is more optimal, and results in expected behavior
		const result = instantiateSync(binary, createImports(imports))

		imports.setExports(result.exports)
		imports.shouldLogAborts(false)

		return result
	},

Can you try this @trusktr ?

@jtenner jtenner added Question ❔ Further information is requested Waiting 🛑 Something is waiting labels Mar 1, 2021
@jtenner jtenner self-assigned this Mar 1, 2021
@trusktr
Copy link
Contributor Author

trusktr commented Mar 3, 2021

as-pect doesn't want to mutate your imports object. It returns a brand new one.

That's true, but it could copy the methods over anyways. But what if there is a number, string, boolean, null, or symbol?

Maybe,

const isObject = value && typeof value === 'object'
finalImports[key] = isObject ? Object.assign(finalImports[key] || {}, value) : value;

@jtenner
Copy link
Contributor

jtenner commented Mar 3, 2021

as-pect copies each namespaced import into a new object using Object.assign(). There is no mutation.

The functions that were being modified were not even being imported into web assembly.

@jtenner
Copy link
Contributor

jtenner commented Apr 28, 2021

Perhaps adding an install method as a separate feature request. It would require a refactor. In the meantime, there are ways around this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question ❔ Further information is requested Waiting 🛑 Something is waiting
Projects
None yet
Development

No branches or pull requests

2 participants