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 Node JS engine support #1021

Closed
wants to merge 12 commits into from
Closed

Add Node JS engine support #1021

wants to merge 12 commits into from

Conversation

dustinsoftware
Copy link
Member

@dustinsoftware dustinsoftware commented Feb 1, 2020

⚠️ This is very WIP ⚠️

Expose a separate package for opting in to a native Node JS engine

Some of this will probably end up getting backported from:
https://github.com/JeringTech/Javascript.NodeJS
https://github.com/DaniilSokolyuk/NodeReact.NET

Working:

  • Render functions (css-in-js, helmet, router)
  • Tests
  • .NET Framework
  • .NET Core
  • Babel / on the fly compilation
  • Engine pooling

Things to consider;

  • If this ends up supporting Netfx, it must ensure that wrapping the async methods does not cause deadlocks. A library fork that doesn't use HttpClient could work as well. Razor doesn't support async at all in Netfx.
  • How much work should be done to support full backwards compatibility with how script execution currently works?
  • Updating the watched files should cause the Node engine to be reloaded
  • Lazy loading is a possibility now, but probably only feasible with a split webpack configuration (target: node and target:web sections)
  • Is cross platform going to be supported at all?
  • How are scripts using UMD supposed to be supported without first being processed with webpack? (reactstrap crashes immediately because require('react') hasn't been resolved to react in the vendor bundle)

The underlying module parser lives here

string Name { get; }
string Version { get; }

bool HasVariable(string uSER_SCRIPTS_LOADED_KEY);
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that u is lowercase? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, the Visual Studio code-generator couldn't decide how to handle the screaming snake case

@Daniel15
Copy link
Member

Daniel15 commented Feb 1, 2020

Good idea! I wonder if we could add script debugging too, in the future.

@Daniel15
Copy link
Member

Daniel15 commented Feb 1, 2020

I think this closes #971

@Taritsyn
Copy link
Contributor

Taritsyn commented Feb 1, 2020

Hello, Dustin and Daniel!

I started making the JavaScriptEngineSwitcher.Node module in last year. For many reasons, I couldn't finish it (one of reasons was adding support for the ClearScript 6.0). Today I decided to publish a draft version. In my version, I don't use eval because of problems with state separation. Instead, I use features of the vm module.

In a couple of weeks, I plan to return to finalizing this module.

@dustinsoftware
Copy link
Member Author

Cool! I'll plan on continuing this experiment to see what's possible using a real Node environment.

Aiming to get feature parity first with the current JS Engine solution (render functions don't work yet, nor does babel compilation) and then expand to things like lazy-loading, better debugging, etc. There may be some additional Node-specific functions that JSEngineSwitcher doesn't yet expose to take advantage of features like lazy-loading. Would love to discuss exposing those functions in JSEngineSwitcher as we run into new Node-specific use cases during this experiment.

Using vm is a good idea to help isolate evaluated Javascript, will give that a look as well...

https://nodejs.org/api/vm.html#vm_script_runinnewcontext_contextobject_options

@dustinsoftware
Copy link
Member Author

cc @DaniilSokolyuk in case you're interested or have suggestions...

@Taritsyn
Copy link
Contributor

Taritsyn commented Feb 2, 2020

… to take advantage of features like lazy-loading.

@dustinsoftware Do you mean support of CommonJS modules and require function?

@dustinsoftware
Copy link
Member Author

Yes; this is necessary to handle bundles webpack emits with target: node, which will defer to the module resolution facilitated by require for both static and dynamic imports

@Taritsyn
Copy link
Contributor

Taritsyn commented Feb 2, 2020

ClearScript library already supports CommonJS modules. For other engines, need to implement it. Most likely, before this, will need to implement a full-fledged Intertop in all modules, and this is not an easy work.

In addition, I have already tried to implement ES modules in ChakraCore. This task will also need to be returned to.

Unfortunately, there is no time yet. In a week or two I plan to go back to working on the Javascript.NodeJS module.

@dustinsoftware
Copy link
Member Author

dustinsoftware commented Feb 2, 2020 via email

@DaniilSokolyuk
Copy link
Contributor

Sync over async can cause thread-pool starvation and results in service outages

@dustinsoftware
Copy link
Member Author

Engine pooling and file watching have landed directly in the library:
JeringTech/Javascript.NodeJS#69

I think by default we will only want to start with one instance running.

Notably we would not want to defer to JsPool for this engine since it would conflict with the library's built in pooling.

@JeremyTCD
Copy link

I think by default we will only want to start with one instance running.

You're right, only 1 instance is required. Enabling pooling/concurrency:

services.Configure<OutOfProcessNodeJSServiceOptions>(options => {
    options.Concurrency = Concurrency.MultiProcess; // Concurrency.None by default
    options.ConcurrencyDegree = 8; // Number of processes. Defaults to the number of logical processors on your machine.
);

@dustinsoftware
Copy link
Member Author

Using JavaScriptEngineSwitcher.Node works well enough, although it uses sync-over-async just like this approach does. I'm going to close this WIP PR and open up smaller PR's for any changes we need before recommending using Node as the default path.

For instance, if node.exe is killed, it will respawn but not have any of the user scripts loaded correctly. The entire app has to be restarted to get the out of this broken state.

image

@dustinsoftware
Copy link
Member Author

Wow; remarkable.

I never thought I'd see the day of this library server-rendering components on a Raspberry pi. Very cool stuff.

image
image

@JeremyTCD
Copy link

Nice! After JeringTech/Javascript.NodeJS#75, I was planning to test Jering.Javascript.NodeJS on my pi. Cool to see that it works in your experiment 😃. Let me know if any issues pop up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants