> but opening almost every JS file twice doesn't make sense to me. We agree completely on this.
-- Josh Tynjala Bowler Hat LLC <https://bowlerhat.dev> On Tue, Aug 20, 2019 at 11:21 AM Alex Harui <[email protected]> wrote: > 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 > > > > > > > > > > > >
