Doesn't your plan assume that searching for a potentially (and probably) missing companion file is faster than opening and scanning a few lines of an known-to-exist file? That's opposite of what I thought was the case.
I haven't looked closely at how things work today, but I thought we were searching every JS file for @extern annotations (as well as remove-circular info) and adding files with @externs to the externs list. Maybe that code no longer runs these days with recent changes to better handle source externs. But also check externs from typedefs SWCs. There may be other ways files get added to the externs list. I'm not opposed to refactoring to improve performance and consider separating dependency on the Closure Compiler, but opening almost every JS file twice doesn't make sense to me. Another idea may be searching the externs as they are being copied to the output folder. Maybe there should be some data structure that various file handlers contribute to. Maybe there should be more Publisher classes with a base class that the Closure Publisher overrides to further scan the files for remove circular dependencies. Maybe I'm too old, but my recollection was that opening files was more expensive than reading opened files so I'm hoping we can avoid doing that. -Alex On 8/20/19, 11:03 AM, "Josh Tynjala" <[email protected]> wrote: 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://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbowlerhat.dev&data=02%7C01%7Caharui%40adobe.com%7Ce071bbe36e6848e8731208d72598c074%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637019210332973330&sdata=0cNEXnLCkz48JYr3RRXgyJj%2FMQzc%2Fp27pCT70ozU8DQ%3D&reserved=0> On Tue, Aug 20, 2019 at 10:31 AM Alex Harui <[email protected]> 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" <[email protected]> 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%7Ce071bbe36e6848e8731208d72598c074%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637019210332973330&sdata=0cNEXnLCkz48JYr3RRXgyJj%2FMQzc%2Fp27pCT70ozU8DQ%3D&reserved=0 > > > > >
