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

feat: support Object.seal object Object.preventExtensions #213

Conversation

sor4chi
Copy link
Contributor

@sor4chi sor4chi commented Oct 30, 2024

Hello! Thank you for this fantastic open-source project!

I was intrigued by Ezno, so I decided to try one of the good-first-issues to study and get familiar with the codebase.


#198

  • update checker type definitions (and generate binary)
  • define the frozen type for multiple freezing ways
  • extend the frozen field of LocalInformation with frozen type

As far as I know, objects that have been Object.freeze, Object.seal, or Object.preventExtensions will all have both Object.isFrozen and Object.isSealed return true. Therefore, I’m using existence checks based solely on frozen HashMap keys.

@kaleidawave kaleidawave added bug Something isn't working checking Issues around checking labels Oct 30, 2024
@kaleidawave
Copy link
Owner

Awesome! Thanks for checking it out and even more for the contribution. Currently a bit busy this afternoon (you will see soon) so will check this out tomorrow or Friday. You can ignore the CI errors for now as they are for separate things that I will fix.

In the meantime you can check out the specification.md file to see how the type checker is "unit tested". If you have any of the new features working right now for seal and preventExtensions and it catches errors not currently presently caught in the main branch it would be good to add some custom tests in the markdown format of specification.md (but put them in the staging.md file for now to keep them separate from the finished features).

Thanks 🙏

@sor4chi sor4chi force-pushed the feat/object-seal-object-prevent-extensions branch from 6846c45 to a8fcb23 Compare October 30, 2024 23:30
@sor4chi sor4chi changed the title feat: support Object.seal object Object,preventExtensions feat: support Object.seal object Object.preventExtensions Oct 31, 2024
@kaleidawave
Copy link
Owner

Awesome! This looks ready to merge. I will have to re-read the MDN documentation on this to double check that everything is correct and whether there is anything else that could be implemented. But looking good, new specification tests passing AND no breakages 🎉 https://github.com/kaleidawave/ezno/actions/runs/11603282627/job/32383499515?pr=213#step:8:323

@kaleidawave kaleidawave added the ready Ready for next release label Nov 1, 2024
@sor4chi
Copy link
Contributor Author

sor4chi commented Nov 3, 2024

I read the MDN and noticed that Object.isExtensible. (false if any of the three fixing methods are used).

I'll implement this as well.

https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Object/isExtensible

@sor4chi
Copy link
Contributor Author

sor4chi commented Nov 4, 2024

How about this?
Now it seems unlikely that there will be any more to add.

@kaleidawave
Copy link
Owner

Awesome! Will merge later.

@kaleidawave kaleidawave merged commit d67cd1b into kaleidawave:main Nov 4, 2024
6 of 9 checks passed
@kaleidawave
Copy link
Owner

Hey! Great work, I like the abstraction with enum ObjectProtectionState and nice work figuring out constant_functions. Made a few changes

  1. Fixed my own mistake as was a single line: in assignment, it was looking at the protection state in the current environment but not ones above (using facts_chain), it now uses the proper method so it should not allow to modifying a frozen object in an above context. For example const x = Object.freeze({}); if (condition) { x.g = 2; } now throws a type error.
  2. The seal allows modifying existing properties, so I changed the logic to only throw a type error if registering a new property (updated the tests to match that behaviour)
  3. Also changed the constant functions to be a little more specification correct

There is still some uncaught edge cases with Object.isFrozen(Object.seal({})) === true because .isFrozen seems to observe existing properties and whether any are configurable. https://natto.dev/@kaleidawave/12d97430628e4e8ebe5408b1e67992a8. Don't fully understand ATM, but should be fine for what I really wanted which is .seal and .preventExtensions

Also did what I do for every PR when they are finished which is to merge the staging tests into the final specification once it is ready! (I should come up with a way to automate that process).

@kaleidawave
Copy link
Owner

Also thanks for the sponsorship 🎉. Next weeks blog post should be about properties, so I will mention this feature in it!

@sor4chi
Copy link
Contributor Author

sor4chi commented Nov 5, 2024

Thank you very much!
The code was very easy to understand and debugging was a breeze.
I will start on the next task (good-second-issue) before this interest dies down!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working checking Issues around checking ready Ready for next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants