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

Chore: Remove ABI from schema package #1073

Merged
merged 28 commits into from
Aug 12, 2022

Conversation

cbrzn
Copy link
Contributor

@cbrzn cbrzn commented Jul 25, 2022

closes #973

  • use undefined instead of null in abi to losslessly save space while encoding to msgpack.
  • replace Abi from schema parse with WrapAbi from wrap manifest.
  • fix a bug in msgpack binding where it sometimes adds an extra 130 in Uint8Array buffer.

@cbrzn
Copy link
Contributor Author

cbrzn commented Jul 29, 2022

If I understand things correctly the change of null to undefined is an improvement of the current structure, right? If that's the case, I would suggest tackling this in two steps, the first entirely removing the Abi definition from schema/parse and then making these improvements - What do you think ser @Niraj-Kamdar

edit: it's worth mentioning that I will follow your lead on this since you're more experienced with this part of the code Niraj, I am just making a suggestion, so feel free to ignore it :P

@Niraj-Kamdar
Copy link
Contributor

@cbrzn We could have but I already finished 1/3 of the work

@cbrzn
Copy link
Contributor Author

cbrzn commented Jul 29, 2022

@cbrzn We could have but I already finished 1/3 of the work

perfect, go for it then bro

@Niraj-Kamdar Niraj-Kamdar marked this pull request as ready for review August 1, 2022 10:01
Copy link
Contributor Author

@cbrzn cbrzn left a comment

Choose a reason for hiding this comment

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

🔥 you rock Niraj - Thanks for pushing this forward

Co-authored-by: Cesar Brazon <cesarbrazon10@gmail.com>
Niraj-Kamdar
Niraj-Kamdar previously approved these changes Aug 8, 2022
envType: createEnvDefinition({
properties: [
createScalarPropertyDefinition({ name: "foo", type: "String", required: true }),
Copy link
Contributor

Choose a reason for hiding this comment

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

@cbrzn can you please disable auto formatting, it bloats the size of PRs and makes it difficult to identify where changes have occurred that are not just styling changes.

@@ -22,7 +22,8 @@
"type": "string",
"enum": [
"wasm",
"interface"
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes should technically be going into a new version of the schema. Since no breaking changes are being made (if I'm correct) the version would be 0.2.

Since this work is fixing things that should have been done before origin release, we can slide these changes in unnoticed right now IMO, but this should be the last time we do something like this.

dOrgJelli
dOrgJelli previously approved these changes Aug 12, 2022
@dOrgJelli dOrgJelli merged commit 19415ba into origin-dev Aug 12, 2022
@dOrgJelli dOrgJelli deleted the chore/remove-abi-from-schema-parse branch April 10, 2023 17:04
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