Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent publishing of empty files #997

Conversation

MikeAlMartinez
Copy link
Contributor

@MikeAlMartinez MikeAlMartinez commented Dec 28, 2023

This PR seeks to fix Issue #981.

The PR adds a config variable pruneEmptyCodeFiles which defaults to false. When set to true, brightscript files that are considered empty won't be published during transpilation. It also adds a canBePruned property to the Brs and XML files.

Currently Brs files are considered empty if they don't contain a FunctionStatement, MethodStatement, or a ClassStatement. I might have missed something here.

I can't really think of any reason why an xml file would be considered empty so I've defaulted the value to true for now. One thing I have implemented here for XML files is, imports of empty scripts are removed. If a brightscript file that an xml files references has their canBePruned field return false, the associated import will be removed too.

On a large internal project, this resulted in significant compile-time speedups.

  • ultra 4800x (12.5):

    • pruning off: 7250ms
    • pruning on: 4965ms
  • stick4k 3800x (12.5):

    • pruning off: 5789ms
    • pruning on: 4342ms
  • express 3960x (11.5):

    • pruning off: 8232ms
    • pruning on: 6111ms

FunctionStatement: () => {
isPublishable = true;
},
MethodStatement: () => {
Copy link
Member

Choose a reason for hiding this comment

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

Methods can't exist on their own, so we can probably remove this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed 👍

isPublishable = true;
}
}), {
walkMode: WalkMode.visitStatementsRecursive
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need the recursive part (that steps into function bodies which we don't care about for this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure if perhaps something like a namespace could contain functions so perhaps I needed the recursive version? Happy to change this and the methods version though. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@TwitchBronBron
Copy link
Member

Okay, so I've been toying with a different name for this option. I'm open to other names, but here's what I came up with:

Let's rename publishEmptyFiles to pruneEmptyCodeFiles and invert the logic and default to false. It's a bit more specific to the action being performed. I also like that the default is to not prune them, and then when you enable pruneEmptyCodeFiles, bsc will start pruning them. Also, the term publish is a bit loaded because sometimes that could mean actually copying to the device (which we aren't doing with this prop).

Along with that, I'm thinking we should rename isPublishable to canBePruned.

Thoughts?

Comment on lines 466 to 471
private determinePkgMapPath(uri) {
if (/^pkg:\/.+/.test(uri)) {
return uri.replace(/^pkg:\//, '');
}
return util.getPkgPathFromTarget(this.pkgPath, uri);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you can completely eliminate this function and just call util.getPkgPathFromTarget from checkScriptsForPublishableImports. We do a similar pkg:/ check in getPkgPathFromTarget.

@MikeAlMartinez
Copy link
Contributor Author

Okay, so I've been toying with a different name for this option. I'm open to other names, but here's what I came up with:

Let's rename publishEmptyFiles to pruneEmptyCodeFiles and invert the logic and default to false. It's a bit more specific to the action being performed. I also like that the default is to not prune them, and then when you enable pruneEmptyCodeFiles, bsc will start pruning them. Also, the term publish is a bit loaded because sometimes that could mean actually copying to the device (which we aren't doing with this prop).

Along with that, I'm thinking we should rename isPublishable to canBePruned.

Thoughts?

That makes sense to me. I wasn't really sold on publish either so will update. 👍

src/BsConfig.ts Outdated Show resolved Hide resolved
src/files/BrsFile.ts Outdated Show resolved Hide resolved
src/files/XmlFile.ts Outdated Show resolved Hide resolved
@TwitchBronBron TwitchBronBron added create-package create a temporary npm package on every commit and removed create-package create a temporary npm package on every commit labels Jan 5, 2024
@TwitchBronBron
Copy link
Member

This is fantastic work! Thank you so much.

@TwitchBronBron TwitchBronBron merged commit 655ff1a into rokucommunity:master Jan 8, 2024
12 of 14 checks passed
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.

None yet

2 participants