Skip to content

Conversation

dustinsoftware
Copy link
Member

This started as an effort to remove VroomJs from the core library, and evolved into removing the default engine registration logic, so the core library can remain simple. The MSBuild project ends up registering the MSIE engine directly, which works fine. I updated a lot of the documentation to make getting started more clear as well.

Previous efforts are in #501, #459 , and #456

Once this is merged we can work on #566 :)

@DanBuild
Copy link
Collaborator

DanBuild commented Aug 13, 2018

Website preview is ready!
Built with commit 0a73071
https://deploy-preview-582--reactnet.netlify.com/

@dustinsoftware
Copy link
Member Author

Ugh.. cassette does not seem to work with .NET 4.5. Going to roll back those changes (should be minor)

@dustinsoftware
Copy link
Member Author

Tested all the samples, they pass. This should be ready for review.

@dustinsoftware
Copy link
Member Author

@Daniel15 can you give this a quick look? I've tested it but it's technically a breaking change (suitable for a 4.x release) since JS engines are no longer registered automatically.

cc @Taritsyn

@Taritsyn
Copy link
Contributor

I'll try to reply in 36 hours.

Copy link
Member

@Daniel15 Daniel15 left a comment

Choose a reason for hiding this comment

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

This looks great to me! We can include this as part of the 4.x release, since we already have another breaking change (switching to newer JsEngineSwitcher beta version) :)

I do still wonder if we should remove the usage of JsEngineSwitcher.Current and instead use the IoC container to manage its life cycle. I guess that'd make registering JS engines a bit more difficult though. Probably not worth it?

title: Webpack
---

This guide is for Webpack 1. To see the latest example for how to use webpack, check out the [sample project](https://github.com/reactjs/React.NET/tree/master/src/React.Sample.Webpack).
Copy link
Member

Choose a reason for hiding this comment

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

We should upgrade this guide one day.

.SetReuseJavaScriptEngines(false);

JsEngineSwitcher.Current.DefaultEngineName = MsieJsEngine.EngineName;
JsEngineSwitcher.Current.EngineFactories.AddMsie();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if a singleton instance of JsEngineSwitcher should be registered in the IoC container rather than using the static Current property.

@Taritsyn
Copy link
Contributor

My suggestions are as follows:

  1. Add a EngineName property to the IReactSiteConfiguration interface. The value of this property when choosing a engine must take precedence over the JsEngineSwitcher.Current.DefaultEngineName. This will avoid conflicts with other software products that use the JavaScript Engine Switcher.
  2. It is possible that we should abandon the automatic selection of a working engine, because it introduces an element of randomness. The user still does not know which engine is currently in use.
  3. If we ignore the second item, then before catching the exception, we should add the following code:
catch (JsEngineLoadException ex)
{
	Trace.WriteLine(string.Format("Error initialising {0}: {1}", engineFactory, ex));
	exceptionMessages.Add(ex.Message);
}

This code intercepts almost all common errors that occur when creating engines. In the Message property describes the reasons and ways to prevent such errors (see example).

@Taritsyn
Copy link
Contributor

I do still wonder if we should remove the usage of JsEngineSwitcher.Current and instead use the IoC container to manage its life cycle. I guess that'd make registering JS engines a bit more difficult though. Probably not worth it?

This can introduce additional confusion.

@dustinsoftware
Copy link
Member Author

Thanks for the feedback. I pushed an update to exception logging. Changing the way the default JS engine is selected seemed out of scope for this PR and I'm not quite sure yet if a web project would need default JS engine separately from the built-in handling in JsEngineSwitcher. As long as jsEngineSwitcher.DefaultEngineName is set, this library won't try to use another engine even if it has been registered.

So with that in mind, I'll merge after the build passes.

@dustinsoftware dustinsoftware merged commit 3281a16 into reactjs:master Aug 20, 2018
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.

4 participants