Avoid parsing JS files when building a bundle

[niloc132]

This avoids the case where LazyParsedDependencyInfo.getLoadFlags() might parse the entire file to check if a JS file can be bundled as a Closure or ES6 module.

Fixes #4006

[lauraharker]

I'm trying to figure out why @googlebot hasn't run for this PR - usually it automatically runs to check if you've signed a Contributor's Licensing Agreement. Example: #3833 (comment).

I'll try to retrigger @googlebot. Meanwhile, if you haven't already signed at https://cla.developers.google.com/, please go ahead and do so.

[lauraharker]

Ok, I reran the CLA check and looks like you've already signed, thanks.

I've imported this for some internal testing.

[lauraharker]

Should isModule() and isGoogModule() always return the same thing?

If so let's be more explicit about that - could we make DependencyInfo.Base.isModule final? The only overrides I see are in JSChunk.java and DependencyMap.java.

[niloc132]

Probably, but I'm not totally sure that it is always the same thing. From JSChunk.isModule():

  @Override
  public boolean isModule() {
    // NOTE: The meaning of "module" has changed over time.  A "JsChunk" is
    // a collection of inputs that are loaded together. A "module" file,
    // is a CommonJs module, ES6 module, goog.module or other file whose
    // top level symbols are not in global scope.
    throw new UnsupportedOperationException();
  }

I was concerned that, despite the implementation in DependencyInfo.Base.isModule() explicitly checking for goog modules, that this JsChunk implementation required that isModule continue to throw. The comment seems to be generally inaccurate, or at least in conflict with other implementations, so I only changed DependencyInfo.Base.isModule() to take the fast path (and skip parsing), but stil permit JSChunk to throw in this specific case.

I didn't notice at the time (but am seeing now) that JSChunk also throws for getLoadFlags(), so perhaps I'm being overly cautious. Will be guided by your knowledge/preference here - if I'm being too careful about code that I can't see (that assumes isModule() might in fact be referring to commonjs/es6/goog as the comment describes), yes, we can probably delete JSChunk.isModule().

--

I can see no source file with the name DependencyMap.java, nor type with that name - typo, or internal class I can't see?

[lauraharker]

I'm reasonably confident that the JSChunk comment is just misleading, and isModule should really mean isGoogModule.

that comment and the isModule method were both added back in 2014 in 57bbf72. That commit is adding goog.module support. At the time, Closure Compiler already had some support for CommonJS modules (5ef7a8a) but not ES modules.

If you're willing to completely replace isModule with isGoogModule that sounds good to me. I don't see too many usages even if I include internal-only classes.

---

DependencyMap.java is an internal class yes, my bad. I'll just update it when importing your change.

[niloc132]

Removed that method, only found in tests, a javadoc comment, and one invocation that didn't record the value, and may have been a mistake. Along the way I removed the Base abstract class as it only had one method left, and pushed that up to the interface as a default method - at least in the public git repo, there are no overloads of it, so probably minimal impact.

I think this can be resolved, but will leave it open for you to confirm.

[lauraharker]

I'm reasonably confident that the JSChunk comment is just misleading, and isModule should really mean isGoogModule.

that comment and the isModule method were both added back in 2014 in 57bbf72. That commit is adding goog.module support. At the time, Closure Compiler already had some support for CommonJS modules (5ef7a8a) but not ES modules.

If you're willing to completely replace isModule with isGoogModule that sounds good to me. I don't see too many usages even if I include internal-only classes.

---

DependencyMap.java is an internal class yes, my bad. I'll just update it when importing your change.

[lauraharker]

Update: I'm looking at a few test failures in our internal test suite. Currently investigating whether they're caused by some internal-only changes I made/failed to make, or something in this PR.

[lauraharker]

Another update:

I'm currently working on trying to avoid using the regex parser for compilations that need to fully parse each input file anyway. So the goals of these two changes don't conflict, but I still need to verify my implementation won't mess up your PR or vice versa.

[niloc132]

Thanks for the update, I appreciate it. Let me know if it makes more sense to merge your changes and have me try to rework this instead of you managing both.

We're trying to pick up the typed_ast feature for ADVANCED compiles without re-parsing everything on small changes, but have a sort of simplified version for BUNDLE that serializes the results of the regex parser to avoid even redoing that parsing work on the bulk of a given library. While this is working, we're still figuring out typed_ast, so redoing some of this work wouldn't be a big impediment if it helps generally improve performance.

[lauraharker]

Thanks for all your patience - this change looks good to go, no conflicts with what I'm working on. I've gotten it approved in internal code review and plan to submit it on Monday.