Proposal for improvements of electron-edge-js

[Hammster]

Hi,

I've been working with @peter-sabath on creating a new Pull Request with a list of improvements and bugfixes. and we had a few discussions about how to improve electron-edge-js and edge-js

Proposal

• Merge electron-edge-js and edge-js into one library
• Setup a CI (via github actions) and make use of prebuilds that are only distributed in the needed version currently you have to get all versions Packaging for Windows from Mac or Linux #20 Prebuild #64
• Improve the code quality
  • Most scripts in /Tools are MS only and/or are outdated/not-working
  • Currently tabs/spaces, semicolons, quotes are over the place 🙂

Optional Proposal

• Change the current JS codebase to TypeScript
• Make the edge-{langname} compiler optional
  • Currently the package always pull in edge-cs as a dependency and this could be opt-in
• Create some kind of chat channel discord/slack for the users and also to make it easier for users and contributors to communicate 😄

List of the changes we made:

• BigInt Support [CLRnative/Mono]
• Faster Marshalling [CLRnative/Mono]
• Optional Object Converter Interface for [CLRnative/Mono]
• Fixed Object/Field/Property visibility for [Mono]
• Type Information Cache [Mono]
• Fix for struct/property/field access [Mono]
• Fixed Linting [JS]
• ES6-ified the script [JS]

[yongjias]

sounds great

[Hammster]

Also, to boil down the actually code differences (Excluding scripts) from electron-edge-js and edge-js

It's these lines:

Probably just never got updated to .serialize() instead of toString()

electron-edge-js/src/CoreCLREmbedding/host/runtime_config.cpp

Line 41 in f7a5683

: property.second.to_string();

Also appears to be a missing update

electron-edge-js/src/double/Edge.js/dotnet/EdgeJs.cs

Lines 22 to 23 in f7a5683

	// needed because we *must* load the edgedll from the original location else we'll freeze up.
	edgeDirectory = Path.GetDirectoryName(System.Reflection.Assembly.GetAssembly(typeof(Edge)).Location);

This one should not be a issue after using prebuild

electron-edge-js/lib/edge.js

Lines 6 to 19 in f7a5683

	var versionMap = [
	[ /^6\./, '6.5.0' ],
	[ /^7.4\./, '7.4.0' ],
	[ /^7.9\./, '7.9.0' ],
	[ /^7\./, '7.9.0' ],
	[ /^8.2\./, '8.2.1' ],
	[ /^8.9\./, '8.9.3' ],
	[ /^8\./, '8.9.3' ],
	[ /^10.2\./, '10.2.0' ],
	[ /^10.11\./, '10.11.0' ],
	[ /^10\./, '10.11.0' ],
	[ /^12.0\./, '12.0.0' ],
	[ /^12.4\./, '12.4.0' ],
	];

And this one seams to do nothing at all, there is no further checking and it would be forced into a not found issue when not set.

electron-edge-js/lib/edge.js

Lines 60 to 62 in f7a5683

	if (process.versions['electron'] || process.versions['atom-shell'] || process.env.ELECTRON_RUN_AS_NODE) {
	edge = require(edgeNative);
	}

[agracio]

@peter-sabath @Hammster
First wanted to thank you guys for making this project better!
Have a few comments in no particular order or importance.

• Not convinced about merging the code, electron-edge-js can be very finicky when using on with different Electron versions and systems, would be easier to troubleshoot.
• Code cleanup is always welcome.
• There is very little JS code in the project, don't think it should be moved to TypeScript (and I myself use Typescript exclusively) there is no need to add complexity.
• Removing edge-cs dependency will disable C# support don't see how it could be an opt-in.
• This line is actually very important for Electron process to work, although I cannot remember why right now.
  

  electron-edge-js/lib/edge.js

  Lines 60 to 62 in f7a5683

  	if (process.versions['electron'] || process.versions['atom-shell'] || process.env.ELECTRON_RUN_AS_NODE) {
  	edge = require(edgeNative);
  	}

• There is already a Slack room for Edje.js it's on top of the original Edje.js README ;)
  Edge.js is now on Slack at https://edgejs.slack.com. Join here.

These are just a few random comments without going in depth.