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: Add utilities#assert [#95469256] #309

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alexanderGugel
Copy link
Member

Opened as PR for code review. Initially opened as issue #285


DOMRenderer previously had methods for asserting state.

Putting those methods on the prototype makes it impossible for the
minifier to do any kind of dead-code elimination.

This commit adds an assert utility function used in order to check
if a certain condition is met.

Basically we are probably going to have an assert method that checks if a certain condition is being met and throw an error otherwise (basically like t.ok in tape). The issue is to make this minifiable/ integrate with our existing tools.

Feedback is very welcome. The main goal is to

  1. avoid unnecessary function calls (micro optimization - yay!)
  2. have the ability to remove error messages for minification purposes.
  3. avoid being locked into browserify by adding an optional transform steps.

@michaelobriena @DnMllr

@jd-carroll
Copy link
Contributor

I think one of the best segments you guys had at jquerysf were the AST talks. Which has made me think a lot about how I could use AST and transformations such as this in my own practice.

Which brings me to....

What is happening in this transformation? Without the browserify transformation you will just bring in the new utilities class assert to challenge the objects. What is gained by adding the browserify transformation?

Let me build this branch and see what the code looks like.

@alexanderGugel
Copy link
Member Author

What is happening in this transformation? Without the browserify transformation you will just bring in the new utilities class assert to challenge the objects. What is gained by adding the browserify transformation?

Everything works perfectly fine without the transform. The transform basically optimizes the code by

  1. avoiding the additional function call to assert by asserting the state before (perf optimization).
  2. stripping error messages from the final bundle to decrease file size (not a perf optimization).

It is also worth noting that while the script is primarily intended as a browserify transform, it simply returns a stream that can be used with arbitrary build tools.

(https://github.com/Famous/engine/pull/309/files#diff-4f98559054de977c5dc700f456ec2185R12)

@alexanderGugel
Copy link
Member Author

Rebased.

DOMRenderer previously had methods for asserting state.

Putting those methods on the prototype makes it impossible for the
minifier to do any kind of dead-code elimination.

This commit adds an assert utility function used in order to check
if a certain condition is met.

Checking for NODE_ENV and not including the passed in error message
in the thrown error should be trivial and only needs to be done once
in the more generic assert function.
@alexanderGugel
Copy link
Member Author

Rebased.

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.

2 participants