feat(stdlib): Implement roAppInfo #643

vbuchii · 2021-04-12T10:40:18Z

fixes #537

lkipke

Looking good so far, a few comments

src/brsTypes/components/RoAppInfo.ts

test/e2e/resources/components/roAppInfo.brs

lkipke · 2021-04-19T19:10:56Z

src/extensions/Process.ts

@@ -40,4 +42,20 @@ export const Process = new RoAssociativeArray([
            },
        }),
    },
+    {
+        name: new BrsString("setManifest"),


I like having extensions to customize testing, but are we just creating this function to enable e2e testing? I'm not sure there's another use-case for needing to dynamically change the manifest location (but I could also be wrong!).

For writing an e2e test, we can change the root property when executing a file. For example, the function tr looks for a file called en_US/translations.ts in the root directory. So when we're testing tr, we change root to be the necessary directory:

brs/test/e2e/Localization.test.js

Lines 21 to 24 in 35d45df

await execute([resourceFile("components", "localization", "source", "main.brs")], {

...outputStreams,

root: "test/e2e/resources/components/localization",

});

Then when we call tr from Brightscript, it finds the correct file:

brs/test/e2e/resources/components/localization/source/main.brs

Lines 1 to 10 in 35d45df

sub main()

_brs_.process.setLocale("en_US")

print tr("Hello") ' => Bonjour

print tr("hello") ' => hello

print tr("Fare thee well") ' => Au revoir

obj = parseJson(tr("Not a straightforward string"))

print obj.hello[0] ' => "world"

print obj.foo ' => 123

end sub

I agree with Levi, let's use above suggestion if this was only set to support E2E testing

Hi @lkipke, @alimnios72
Thank you for your feedback. I did try to use a similar approach but was unable to reach the needed result. Maybe it because of using the internal cwd() function. Then I found in the codebase set locale function and decided to use that approach.

I added changes to avoid using the additional function, please review 😊

alimnios72

I left a few comments below but overall this looks pretty good!

src/brsTypes/components/RoAppInfo.ts

alimnios72 · 2021-04-20T00:50:05Z

src/extensions/Process.ts

@@ -40,4 +42,20 @@ export const Process = new RoAssociativeArray([
            },
        }),
    },
+    {
+        name: new BrsString("setManifest"),


I agree with Levi, let's use above suggestion if this was only set to support E2E testing

alimnios72 · 2021-04-20T00:57:57Z

src/brsTypes/components/RoAppInfo.ts

+        impl: (interpreter: Interpreter) => {
+            let title = RoAppInfo._manifest.get("title");
+
+            return title !== undefined ? new BrsString(title.toString()) : new BrsString("");


here and in below function you could use title != null .... which will check for null and undefined, otherwise if the value was null if will fail that check

done, thanks

src/brsTypes/components/RoAppInfo.ts

… func

sjbarag

We're definitely getting closer here! I'd like to avoid re-parsing the manifest file if possible, but my bigger concern is around the use of process.cwd() in a static property of RoAppInfo — that won't work for our end-to-end tests (which you worked around by manually assigning to a private field), and will subtly break other use-cases as they arise.

Do you happen to know which manifest file is used when createObject("roAppInfo") is called from a ComponentLibrary? I imagine that will complicate things here a bit 😃

src/brsTypes/components/RoAppInfo.ts

sjbarag · 2021-04-20T17:10:36Z

test/e2e/BrsComponents.test.js

+        manifest = preprocessor.getManifestSync(
+            process.cwd() + "/test/e2e/resources/conditional-compilation"
+        );
+        RoAppInfo.manifest = manifest;


Watch out! Since this value is never reset, any test that requires an accurate RoAppInfo.manifest after this will fail in a very unexpected way. There's likely a way to solve this problem without explicitly reading an arbitrary manifest as part of this test (more details in a separate comment).

sjbarag · 2021-04-20T17:13:38Z

src/brsTypes/components/RoAppInfo.ts

+export class RoAppInfo extends BrsComponent implements BrsValue {
+    readonly kind = ValueKind.Object;
+
+    private static manifest = PP.getManifestSync(process.cwd());


Passing process.cwd() will introduce some very subtle bugs when the root directory for brs isn't process.cwd() — it often isn't in the end-to-end tests in this project!

Since we parse the manifest as basically the first step of this project (before brightscript files are even loaded!) I'm curious if we could re-use the parsed manifest, possibly by storing it as a property on the Interpreter instance that gets created early-on?

Thanks @sjbarag
I've introduced the manifest as property on the Interpreter instance as you suggested. So, we no longer re-parse manifest and don't use process.cwd().

vbuchii · 2021-04-21T17:49:35Z

We're definitely getting closer here! I'd like to avoid re-parsing the manifest file if possible, but my bigger concern is around the use of process.cwd() in a static property of RoAppInfo — that won't work for our end-to-end tests (which you worked around by manually assigning to a private field), and will subtly break other use-cases as they arise.

Do you happen to know which manifest file is used when createObject("roAppInfo") is called from a ComponentLibrary? I imagine that will complicate things here a bit 😃

I was unable to find any related info about that in the Roku SDK. So, I've tested on a sample and seen that it is used the manifest file from root of the project. In other words, regardless of where the object is created, the main manifest is used.

sjbarag

Great work, @vbuchii ! Thanks for the improvements 😃

sjbarag · 2021-04-30T23:02:53Z

src/interpreter/index.ts

@@ -94,6 +97,14 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
        return this._environment;
    }

+    get manifest() {
+        return this._manifest != null ? this._manifest : new Map<string, ManifestValue>();


Ooooh returning an empty map is a great idea!

537: implement RoAppInfo

ed96794

vbuchii marked this pull request as draft April 12, 2021 10:42

vbuchii added 3 commits April 13, 2021 15:41

537: add UTs

23e8a6f

537: add e2e tests

82f354a

537: fix lint errors

1bf0112

vbuchii marked this pull request as ready for review April 13, 2021 13:44

vbuchii changed the title ~~537: implement RoAppInfo~~ Issue #537: implement RoAppInfo Apr 13, 2021

lkipke reviewed Apr 14, 2021

View reviewed changes

src/brsTypes/components/RoAppInfo.ts Outdated Show resolved Hide resolved

src/brsTypes/components/RoAppInfo.ts Outdated Show resolved Hide resolved

test/e2e/resources/components/roAppInfo.brs Show resolved Hide resolved

lkipke requested review from sjbarag and alimnios72 April 14, 2021 23:45

lkipke added e2e Affects this project's end-to-end test cases (the BrightScript sample files executed during testing) enhancement Any addition to this project's existing capabilities stdlib Affects the standard library included with this BrightScript implementation labels Apr 14, 2021

vbuchii added 3 commits April 15, 2021 20:50

537: refactor getVersion()

ba49c95

537: make 'manifest' as static variable

ee97070

537: update e2e to use manifest file

a16c762

vbuchii requested a review from lkipke April 17, 2021 19:28

lkipke reviewed Apr 19, 2021

View reviewed changes

alimnios72 reviewed Apr 20, 2021

View reviewed changes

vbuchii added 2 commits April 20, 2021 17:29

537: resolving comments (removed setManifest(), used null, documented…

50cdf4d

… func

537: fix code style

22226eb

vbuchii requested review from alimnios72 and lkipke April 20, 2021 14:36

537: remove unused import

140d639

sjbarag requested changes Apr 20, 2021

View reviewed changes

537: store manifest as property on Interpreter instance

f980ec6

vbuchii requested a review from sjbarag April 21, 2021 17:49

sjbarag approved these changes Apr 30, 2021

View reviewed changes

sjbarag changed the title ~~Issue #537: implement RoAppInfo~~ feat(stdlib): Implement roAppInfo Apr 30, 2021

sjbarag merged commit 4d24b7c into sjbarag:main Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stdlib): Implement roAppInfo #643

feat(stdlib): Implement roAppInfo #643

vbuchii commented Apr 12, 2021 •

edited by sjbarag

Loading

lkipke left a comment

lkipke Apr 19, 2021

alimnios72 Apr 20, 2021

vbuchii Apr 20, 2021

alimnios72 left a comment

alimnios72 Apr 20, 2021

alimnios72 Apr 20, 2021

vbuchii Apr 20, 2021

sjbarag left a comment

sjbarag Apr 20, 2021

vbuchii Apr 21, 2021

sjbarag Apr 20, 2021

vbuchii Apr 21, 2021

vbuchii commented Apr 21, 2021

sjbarag left a comment

sjbarag Apr 30, 2021

	await execute([resourceFile("components", "localization", "source", "main.brs")], {
	...outputStreams,
	root: "test/e2e/resources/components/localization",
	});

	sub main()
	_brs_.process.setLocale("en_US")
	print tr("Hello") ' => Bonjour
	print tr("hello") ' => hello
	print tr("Fare thee well") ' => Au revoir

	obj = parseJson(tr("Not a straightforward string"))
	print obj.hello[0] ' => "world"
	print obj.foo ' => 123
	end sub

feat(stdlib): Implement roAppInfo #643

feat(stdlib): Implement roAppInfo #643

Conversation

vbuchii commented Apr 12, 2021 • edited by sjbarag Loading

lkipke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimnios72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjbarag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbuchii commented Apr 21, 2021

sjbarag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbuchii commented Apr 12, 2021 •

edited by sjbarag

Loading