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

Split up the execution code into multiple files #110

Closed
wants to merge 2 commits into from

Conversation

arduano
Copy link
Collaborator

@arduano arduano commented Mar 23, 2024

Depends on #109

Split up the execution code to be in multiple files, rather than concerning 1 file with like 5 different tasks.

This should make future changes/contributions a lot easier to make and review.

Also, I disabled -Dwarnings in the flake. I assume the intention was to have warnings throw errors in CI, but that's better done with --deny "warnings" instead.

@arduano
Copy link
Collaborator Author

arduano commented Mar 23, 2024

FYI the next plans I have:

  • Refactor the way execution dispatch works, making it much easier to have a shared execution environment that a module could be executed in directly to get the nixjs expression result
  • Use the above to make an import builtin, which evaluates a file/expression into an expression result and returns it
  • Do some cleanups around other parts, e.g. emitting js could be done much cleaner using write! expressions
  • Add some helper abstractions around v8 interaction

@urbas urbas deleted the branch urbas:inline-nixjs-rt March 23, 2024 08:19
@urbas urbas closed this Mar 23, 2024
@arduano arduano mentioned this pull request Mar 23, 2024
@urbas
Copy link
Owner

urbas commented Mar 23, 2024

Oh, I see what happened. This pull request was targeted to the inline-nixjs-rt branch rather than the master branch. When I merged the inline-nixjs-rt branch into master I also deleted the inline-nixjs-rt branch (that's what I always do, to not accumulate branches). The act of deleting the inline-nixjs-rt branch closed this PR (rather than preventing the deletion of the branch).

@arduano
Copy link
Collaborator Author

arduano commented Mar 23, 2024

Yeah there should be an option to re-link the PR to master when you merge the other branch into master iirc

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