Skip to content

Conversation

@hansenyy
Copy link
Contributor

@hansenyy hansenyy commented Nov 1, 2019

This PR moves ScriptStore and RuntimeHolder from RN to RNW. See also microsoft/react-native-macos#185.

Microsoft Reviewers: Open in CodeFlow

@hansenyy hansenyy requested review from a team, acoates-ms and mganandraj November 1, 2019 20:41
@hansenyy hansenyy requested a review from a team as a code owner November 1, 2019 20:41
@ghost ghost added the vnext label Nov 1, 2019
@hansenyy hansenyy requested a review from NickGerleman November 1, 2019 20:41
Copy link
Member

@tudorms tudorms left a comment

Choose a reason for hiding this comment

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

:shipit:

#include <jsi/jsi.h>
#include <memory>

namespace facebook {
Copy link

Choose a reason for hiding this comment

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

{ [](start = 19, length = 1)

(nit) why not use latest faceboo::jsi syntax for namespaces?

namespace facebook {
namespace jsi {

// An instance of this interface is expected to
Copy link

Choose a reason for hiding this comment

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

// [](start = 0, length = 2)

(nit) We should have proper documetation standard at least for new code, eg, doxygen. So comments would be:
//!

// a. lazily create a JSI Runtime on the first call to getRuntime
// b. subsequent calls to getRuntime should return the Runtime created in (a)

// Note :: All calls to getRuntime() should happen on the same thread unless you are sure that
Copy link

Choose a reason for hiding this comment

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

the same thread [](start = 54, length = 15)

(nit) do we care to have code that asserts this?

// JSI::Runtime implementation must be provided an instance on this interface to enable version sensitive capabilities
// such as usage of pre-prepared javascript script. Alternatively, this entity can be used to directly provide the
// Javascript buffer and rich metadata to the JSI::Runtime instance.
struct ScriptStore {
Copy link

Choose a reason for hiding this comment

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

ScriptStore [](start = 7, length = 11)

Aren't we planning to get rid of this per discussion last week?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This PR is just moving RuntimeHolde.h and ScriptStore.h from RN to RNW and I am planning on making more changes here. I'll also fix the nit comments then.


In reply to: 341760394 [](ancestors = 341760394)

@NikoAri
Copy link

NikoAri commented Nov 1, 2019

#include

(nit) can we alphabetize these?


Refers to: vnext/ReactUWP/Utils/UwpPreparedScriptStore.h:7 in 03f1b03. [](commit_id = 03f1b03, deletion_comment = False)

#include <jsi/RuntimeHolder.h>
#include <jsi/ScriptStore.h>
#include <JSI/Shared/RuntimeHolder.h>
#include <jsi/Shared/ScriptStore.h>
Copy link

Choose a reason for hiding this comment

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

jsi/ [](start = 10, length = 4)

(nit) please be consistent in casing

Copy link

@NikoAri NikoAri left a comment

Choose a reason for hiding this comment

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

:shipit:

@hansenyy
Copy link
Contributor Author

hansenyy commented Nov 1, 2019

#include

This is strange. I thought yarn format should have already done this?


In reply to: 548959995 [](ancestors = 548959995)


Refers to: vnext/ReactUWP/Utils/UwpPreparedScriptStore.h:7 in 03f1b03. [](commit_id = 03f1b03, deletion_comment = False)

@hansenyy hansenyy merged commit 2459847 into microsoft:master Nov 1, 2019
@hansenyy hansenyy deleted the moveScriptStore branch November 1, 2019 22:13
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