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

Dependency scanning does not support import statements on more than one line #2559

Closed
rahbari opened this issue Jul 6, 2017 · 17 comments
Closed
Assignees

Comments

@rahbari
Copy link

rahbari commented Jul 6, 2017

I just updated to 20170626, and I get this error while compiling:

ERROR - required "...." namespace never provided

20170521 works just fine.

@ChadKillingsworth
Copy link
Collaborator

I thought we got this fixed ...

@MatrixFrog
Copy link
Contributor

If you can give us a small repro case that would be quite helpful.

@kranich
Copy link
Contributor

kranich commented Jul 7, 2017

I have encountered the same problem. As far as I can tell this happens whenever there is a circular dependency, e.g. two modules importing each other.

@brad4d
Copy link
Contributor

brad4d commented Jul 7, 2017

Probably @ChadKillingsworth is the best person to fix this, but we can't really do anything without an example to reproduce the problem.
@rahbari and @kranich , could either of you provide one?

@ChadKillingsworth
Copy link
Collaborator

I may have an example - I can test it on https://github.com/PolymerLabs/polymer-closure-sample

@kranich
Copy link
Contributor

kranich commented Jul 7, 2017

Another (non-minimal) example would be https://github.com/kranich/ComplexCurves/tree/closure-compiler-20170626. I will see if I can come up with a minimal failing example.

@kranich
Copy link
Contributor

kranich commented Jul 7, 2017

Now I have a minimal (?) failing example at https://github.com/kranich/closure-compiler-issue-2559.

@kranich
Copy link
Contributor

kranich commented Jul 7, 2017

In my case, the issue seems to be that v20170626 no longer allows to spread import statements across multiple lines.

@ChadKillingsworth
Copy link
Collaborator

@concavelenz I believe had a plan for supporting that. @blickly have any suggestions in the mean time?

@ChadKillingsworth ChadKillingsworth changed the title ES6 Module Loading Problem in 20170626 Dependency scanning does not support import statements on more than one line Jul 7, 2017
@blickly
Copy link
Contributor

blickly commented Jul 7, 2017

Looks like this is a known limitation of the regexp-based dependency parsing that we do in JsFileParser. It seems like the best fix would be to make the regexp-parser smarter, like maybe by processing an entire statement at a time (up to a ';') instead of a line at a time (up to a '\n').

In the meantime, the best workaround I can see is either:

  • Adding an extra side-effect only import, which js-beautify doesn't wrap: import './Baz';
    or
  • Manually specify the file order to Closure Compiler so that it doesn't have to do the reordering

@ChadKillingsworth
Copy link
Collaborator

A semi-colon isn't guaranteed though. You almost need a mini-parser there.

So externally we either need to go back to parsing every file for dependency info, or fix the regex parsing. This will break multiple projects.

@blickly
Copy link
Contributor

blickly commented Jul 7, 2017

Yeah, it's complicated. I think that the main reason for needing to do the regex based dependency "parsing" is that we have a few really huge projects that use manage_closure_dependencies and will blow up if all their input files are actually parsed with a real parser. For the ordering of files (as opposed to pruning), I'd think we could always use the more accurate full-parse based information without causing issues there.

Adding @shicks in case he has other ideas.

@blickly
Copy link
Contributor

blickly commented Jul 7, 2017

The other option, of course, is to do what we were (unintentionally) doing before and do a full parse for any file that contains an ES6 module before doing the pruning. I doubt that there are enough ES6 module uses yet for this to cause performance problems, but it seems less than ideal.

@ChadKillingsworth
Copy link
Collaborator

There is another option (for strict dependency modes at least): Parse the entry points and follow the path. Discard any files not reachable. This is very easy with ES6 and CommonJS modules as they are path based. Not sure how you'd approach goog.require/goog.provide code. I suppose you could do regex matching for those.

@ChadKillingsworth
Copy link
Collaborator

After thinking about this for a couple of days, here's what I propose:

If dependency management is enabled:

  1. In order, do a full parse of the first entry point and get correct dependencies.
  2. Parse each dependency discovered from the entry point - ordering the files during the parsing. This should be done depth first.
  3. Move back to step one for the next entry point.

After traversal, discard any unreached files.

As I mentioned during the last meeting, we'll need a full parse to recognize the new dynamic import statement as it can occur anywhere. And yes, we wish to recognize this at build time.

@blickly
Copy link
Contributor

blickly commented Jul 10, 2017

This sounds like a good strategy to me for --dependency_mode=STRICT.

I guess that supporting goog.module/goog.require/goog.provide and --dependency_mode=LOOSE will still require us to keep using the regex-based "parser" for at least some of the cases, but it would mean a big savings for projects with STRICT dependency pruning and no live goog.module/goog.provide files.

@ChadKillingsworth
Copy link
Collaborator

Closed by #2641

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

No branches or pull requests

6 participants