Component Library #274
Component Library #274
Conversation
|
Depends on: |
There was a problem hiding this comment.
Thanks for this! We can chat more about some of my comments in our call on Monday, but there are definitely a few things that I think we can consolidate and simplify.
One thing I'd recommend you investigate before then is the packageTask option. You use that task to build your publishable artifact, and then the the existing sideload flows to install and debug it.
You might not even need any of this "component library" changes, and might only need a few small tweaks to make debugging a little more sane.
I'm going off memory here, but I think your launch.json would look something like this:
{
"type": "brightscript",
"request": "launch",
"name": "BrightScript Debug: Launch",
"stopOnEntry": false,
"rootDir": "${workspaceFolder}",
"packageUploadOverrides": {
//set app_type, roku-deploy already supports this prior to your PR
"formData": {
"app_type": "dcl"
}
},
//this is the name of a task defined in ".vscode/tasks.json"
"packageTask": "build-component-library",
//this is the path to the thing we should upload
"packagePath": "${workspaceFolder}/yourComponentLibrary.zip",
},
RokuDevTools
force-pushed
the
sideload-libraries
branch
from
cf77025 to
8e628ed
Compare
There was a problem hiding this comment.
I've added several comments for things that need some attention.
One thing I saw many places throughout the code was that it's incorrect to assume that the existence of libType means a specific thing. We should always be checking for the value of libType (libType === 'channelstore' or libType === 'remote').
| * if not set the default is false unless one of the libraries requires it | ||
| * @default true | ||
| */ | ||
| prepareProjectFilesSynchronously?: boolean; |
There was a problem hiding this comment.
Let's rename this property to use the term sequentially.
"synchronously" has a different meaning in nodejs, and implies that there won't be any async/await deferment to the nodejs event loop. But that's not how this property works, as there is async code flowing.
The intention behind this property is "prepare each project one-at-a-time" (i.e. sequentially).
| */ | ||
| raleTrackerTaskFileLocation: string; | ||
|
|
||
| libType?: 'channelstore' | 'remote' | 'other'; |
There was a problem hiding this comment.
Do we need the term other here? we don't have code that targets 'other', and the default value is 'remote' unless specifically setting to 'channelstore'. I think we can remove 'other'.
| ]); | ||
| packageEnd(); | ||
|
|
||
| // synchronously prepare the projects based on configuration |
There was a problem hiding this comment.
| // synchronously prepare the projects based on configuration | |
| // sequentially prepare the projects |
| packageEnd(); | ||
|
|
||
| // synchronously prepare the projects based on configuration | ||
| if (this.launchConfiguration.prepareProjectFilesSynchronously) { |
There was a problem hiding this comment.
I'm not entirely sure why we would need this functionality? What is the problem this is trying to solve? If you have 1 main project, and 3 component libraries, why would building the main project first, then all component libraries in parallel solve anything? Can you explain the use case?
There was a problem hiding this comment.
Also, I believe the prepareAndHostComponentLibraries function will still publish component libraries in parallel. That might cause race conditions if multiple component libraries are all trying to run rokuDeploy.publish at exactly the same time to the same device. (unless the Roku installer somehow handles these in parallel?)
| enhanceREPLCompletions: this.launchConfiguration.enhanceREPLCompletions | ||
| }; | ||
|
|
||
| if (componentLibrary.libType) { |
There was a problem hiding this comment.
Need to check for a specific value of libType. This would run if the user set libType to 'channelstore', or 'remote', or even 'chipmunk' because it's only checking for the existance of libType.
There was a problem hiding this comment.
I'm also not sure we need to only set these params when libType === 'channelstore', might as well just set them every time? (it doesn't hurt, as long as there aren't typescript compile issues).
| public get postfix() { | ||
| return `${componentLibraryPostfix}${this.libraryIndex}`; | ||
|
|
||
| return this.libType? '' : `${componentLibraryPostfix}${this.libraryIndex}`; |
There was a problem hiding this comment.
you can't just check for the existence of libType. valid values are: undefined, 'channelstore', 'remote'. Please update this to check for the specific value you want this functionality to follow.
| } | ||
|
|
||
| public async postfixFiles() { | ||
| if (this.libType) return; |
There was a problem hiding this comment.
Same as above. Please check for a specific value of libType here instead of only checking that it's defined.
| public async publish() { | ||
| if(this.libType){ | ||
| if(this.libType !== 'channelstore') { | ||
| util.log(`No publish for libType: ${this.libType}`) |
There was a problem hiding this comment.
Many devs will be using the 'remote' style of channel store and don't want to see this log all the time, so let's utilize this.logger.debug instead of util.log
| return super.stage(); | ||
| await this.setComponentLibraryName(); | ||
|
|
||
| if(this.libType) { |
There was a problem hiding this comment.
Don't just check for the existence of libType. You need to look for a specific value (i.e. 'channelstore' or 'remote').
| this.stagingDir = s`${this.outDir}/../.roku-deploy-staging`; | ||
| await rd.prepublishToStaging({ | ||
| rootDir: this.rootDir, | ||
| stagingDir: this.stagingDir, | ||
| files: this.files, | ||
| outDir: this.outDir | ||
| }); |
There was a problem hiding this comment.
I don't think we can do this. Unless I'm misunderstanding where you're writing this stuff, this would overwrite the main application's staging directory.
Also, I'm fairly certain the main application's staging dir is not permanent, it can be configured as well, so this isn't dependable anyway.
Can you explain why this was necessary? We'll need to find a different way to resolve this if it's needed at all. I feel like the existing component library staging methods should be enough?
2 participants
No description provided.