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

Add ImportObject::get_namespace_exports #2592

Merged
merged 4 commits into from
Oct 12, 2021
Merged

Add ImportObject::get_namespace_exports #2592

merged 4 commits into from
Oct 12, 2021

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Oct 1, 2021

This allows the contents of an existing namespace to be added to by
extracting an Exports from it, adding to that Exports and then
replacing the existing namespace with the modified Exports.

@Amanieu Amanieu requested a review from syrusakbary as a code owner October 1, 2021 15:12
@Amanieu Amanieu force-pushed the get_namespace_exports branch 2 times, most recently from 068fc1f to 93511a2 Compare October 1, 2021 15:14
This allows the contents of an existing namespace to be added to by
extracting an `Exports` from it, adding to that `Exports` and then
replacing the existing namespace with the modified `Exports`.
@Amanieu Amanieu force-pushed the get_namespace_exports branch from 93511a2 to 35d6c02 Compare October 1, 2021 15:16
Comment on lines +23 to +25
fn as_exports(&self) -> Option<Exports> {
None
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Option? Also, why a default impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backward compatibility with any existing implementations of LikeNamespace outside wasmer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If backwards compatibility was not a concern then I would remove LikeNamespace entirely and use Exports directly in ImportObject.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I got the hack, but it looks good to me 👍

@syrusakbary
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Oct 4, 2021
2592: Add ImportObject::get_namespace_exports r=syrusakbary a=Amanieu

This allows the contents of an existing namespace to be added to by
extracting an `Exports` from it, adding to that `Exports` and then
replacing the existing namespace with the modified `Exports`.

Co-authored-by: Amanieu d'Antras <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 4, 2021

Build failed:

@ptitSeb
Copy link
Contributor

ptitSeb commented Oct 4, 2021

Oh, still fd_rename_path on windows x64 that fails?

@syrusakbary
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Oct 4, 2021
2592: Add ImportObject::get_namespace_exports r=syrusakbary a=Amanieu

This allows the contents of an existing namespace to be added to by
extracting an `Exports` from it, adding to that `Exports` and then
replacing the existing namespace with the modified `Exports`.

Co-authored-by: Amanieu d'Antras <[email protected]>
Co-authored-by: Syrus Akbary <[email protected]>
@syrusakbary
Copy link
Member

Let's try again

bors r+

bors bot added a commit that referenced this pull request Oct 11, 2021
2592: Add ImportObject::get_namespace_exports r=syrusakbary a=Amanieu

This allows the contents of an existing namespace to be added to by
extracting an `Exports` from it, adding to that `Exports` and then
replacing the existing namespace with the modified `Exports`.

Co-authored-by: Amanieu d'Antras <[email protected]>
Co-authored-by: Syrus Akbary <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 11, 2021

Build failed:

@syrusakbary
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Oct 12, 2021
2592: Add ImportObject::get_namespace_exports r=syrusakbary a=Amanieu

This allows the contents of an existing namespace to be added to by
extracting an `Exports` from it, adding to that `Exports` and then
replacing the existing namespace with the modified `Exports`.

Co-authored-by: Amanieu d'Antras <[email protected]>
Co-authored-by: Syrus Akbary <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Build failed:

@syrusakbary
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

@bors bors bot merged commit 877ce1f into master Oct 12, 2021
@bors bors bot deleted the get_namespace_exports branch October 12, 2021 11:53
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.

3 participants