Since externs can also include inject_html, and externs don't have circulars to remove, it would faster to find inject_html in those files using this proposed method.
It's true that remove-circulars already opens and reads the JS files. However, by including the check for inject_html in there, we had code that was more complicated than it needed to be only to gain the optimization of opening and reading JS file a single time. Extracting the inject_html detection makes the GoogDepsWriter code more focused, which means it's less effort for a developer to understand what the code is doing. In my experience, there's a lot to process there to fully understand it all. One fewer thing will help. Additionally, in the future, should we decide to add a separate workflow that uses something other than Closure compiler to create a release build (maybe something involving the newer native JS modules), the inject_html detection code will not be as tightly coupled and can be reused more easily. Finally, in my opinion, the less we need to read generated JS files to search for special strings, the better. It may not be possible to avoid it completely, but this is a place where we can avoid that. -- Josh Tynjala Bowler Hat LLC <https://bowlerhat.dev> On Tue, Aug 20, 2019 at 10:31 AM Alex Harui <aha...@adobe.com.invalid> wrote: > IMO, we have to open and read each JS file anyway for remove-circulars, so > it isn't clear that looking for additional companion files is going to be > faster than scanning an already open file. We could make the inject_html > easier to find by moving it higher up in the JS file. > > My 2 cents, > -Alex > > On 8/20/19, 10:25 AM, "Josh Tynjala" <joshtynj...@bowlerhat.dev> wrote: > > I would like to propose a change to how we handle inject_html. > > Currently, it works like this: We define an <inject_html> tag in an > asdoc > comment. That tag gets copied to a jsdoc comment when we emit JS code. > When > we build an app, we search through the JS files line by line to find > <inject_html>. > > I think that, when we emit JS files, it would be beneficial to emit a > separate file that contains the inject_html content. > > So, you'd get a JS file, like we have now: > > com/example/MyClass.js > > Then, if the class contains inject_html, you'd also get: > > com/example/MyClass.js.html > > This is similar, in a way, to how there's a MyClass.js.map file when > source > maps are enabled. > > With this change, instead of loading MyClass.js and parsing it line by > line > looking for inject_html, we could simply check to see if > MyClass.js.html > exists in the same folder (or in the same SWC). This should help us > detect > inject_html much faster and without needing to try to parse JS files. > > Thoughts? > > -- > Josh Tynjala > Bowler Hat LLC < > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbowlerhat.dev&data=02%7C01%7Caharui%40adobe.com%7Cd6daa4ae9b744b8a195c08d725935cbb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637019187152937419&sdata=gSsOaEcI5J%2BaRg8T28eIJoYniqsZd%2F8BsYGjsavlfTg%3D&reserved=0 > > > > >
