Fix the interaction between absolute path imports and module roots #2343

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.

Sign up for GitHub

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

Jump to bottom

Closed

ChadKillingsworth wants to merge 7 commits into google:master from ChadKillingsworth:js-module-root

Collaborator

ChadKillingsworth commented Mar 1, 2017

The interaction between the --js_module_root flag and absolute imports require('/foo/bar.js') results in the module loader unable to locate the module. This applies equally to all 3 module resolution modes and both CommonJS and ES6 modules.

Fixing this without breaking existing functionality has proven challenging. I'm hopeful this PR actually accomplishes it.

Given a module root of base/, the following modules should properly resolve:

base/mod1.js

module.exports = true;

base/app.js

const mod1 = require('/mod1.js');

googlebot added the cla: yes label

ChadKillingsworth changed the title ~~Fix the interaction between absolute path imports an module roots~~ Fix the interaction between absolute path imports and module roots

brad4d self-assigned this

brad4d requested changes

View reviewed changes

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

		@@ -133,7 +133,19 @@ public ModuleLoader(
		}

		public void setPackageJsonMainEntries(Map<String, String> packageJsonMainEntries) {

Contributor

brad4d Mar 2, 2017

Could you add a javadoc comment to this method saying what the keys and values of packageJsonMainEntries represent?

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

+                  for (Map.Entry<String, String> packageJsonMainEntry : packageJsonMainEntries.entrySet()) {
+                    String entryKey = packageJsonMainEntry.getKey();
+                    // For node_modules paths only, remove any leading slash

Contributor

brad4d Mar 2, 2017

Could you expand this comment to explain why it's important to remove the leading slash only for node modules?

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

+                    // For node_modules paths only, remove any leading slash
+                    if (entryKey.startsWith("/node_modules")) {
+                      entryKey = new InputPathData(packageJsonMainEntry.getKey()).stripLeadingSlash();

Contributor

brad4d Mar 2, 2017

nit: why not just pass entryKey instead of calling getKey() again?

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

+                  private final String[] browserFileExtensionsToSearch = {""};
+                  private final String[] nodeFilesToSearch = {
+                    MODULE_SLASH + "package.json", MODULE_SLASH + "index.js", MODULE_SLASH + "index.json"
+                  };

Contributor

brad4d Mar 2, 2017

These should really be "true" constants.
Could you move their definitions outside of ModulePath so you can make the private static final and ALL_CAPS?

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

+                    return !(isAbsoluteIdentifier(this.path) || isRelativeIdentifier(this.path));
+                  }
+                  String ensureLeadingSlash() {

Contributor

brad4d Mar 2, 2017

The method names don't make it clear to me what they do, and I see that sometimes you just need the string but other times you actually need a new InputPathData object with a modified path.

How about this?

InputPathData toRelativePath()
InputPathData toAbsolutePath()

Just add .path to calls when you only need the string.
Or have toRelativePathString() versions if you prefer.

Collaborator Author

ChadKillingsworth Mar 2, 2017

The struggle I have is one of terminology. Within this file:

isAbsoluteIdentifier('/foo') => true
isAbsoluteIdentifier('./foo') => false
isAbsoluteIdentifier('foo') => false
isRelativeIdentifier('/foo') => false
isRelativeIdentifier('./foo') => true
isRelativeIdentifier('foo') => false

I don't know how to label paths that do not begin with either a '.' or '/' as they are neither relative or absolute.

If it's a module path foo/bar.js can either be relative to the compilation root (LEGACY mode), looked up from the node module registry (NODE mode) or invalid (BROWSER mode).

If it's an input path, foo/bar.js is relative to either the compilation or module roots.

It's all horribly confusing.

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

-                    Iterable<String> names, ImmutableList<String> roots) {
-                  HashSet<String> resolved = new HashSet<>();
+                private static ImmutableMap<String, InputPathData> resolvePaths(
+                    Iterable<String> names, ImmutableList<InputPathData> roots) {

Contributor

brad4d Mar 2, 2017

nit: Why not Iterable<InputPathData> roots?

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

-                private static ImmutableSet<String> resolvePaths(
-                    Iterable<String> names, ImmutableList<String> roots) {
-                  HashSet<String> resolved = new HashSet<>();
+                private static ImmutableMap<String, InputPathData> resolvePaths(

Contributor

brad4d Mar 2, 2017

Could you add a Javadoc comment explaining what the keys and values of the return value represent?

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

-                  HashSet<String> resolved = new HashSet<>();
+                private static ImmutableMap<String, InputPathData> resolvePaths(
+                    Iterable<String> names, ImmutableList<InputPathData> roots) {
+                  Map<String, InputPathData> resolved = new HashMap<>();

Contributor

brad4d Mar 2, 2017

nit: use ImmutableMap.builder()

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

-                private static ImmutableSortedMap<String, ImmutableSet<String>> buildRegistry(
-                    ImmutableSet<String> modulePaths) {
-                  SortedMap<String, Set<String>> registry =
+                private static ImmutableSortedMap<String, ImmutableMap<String, InputPathData>> buildRegistry(

Contributor

brad4d Mar 2, 2017

Please expand the JavaDoc to define the meaning of the keys and values of both the input and output.

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

@@ @@ -564,4 +607,30 @@ public String apply(DependencyInfo info) { @@
                    */
                   NODE
                 }
+                private static class InputPathData {

Contributor

brad4d Mar 2, 2017

Need javadoc for this.

ChadKillingsworth force-pushed the js-module-root branch 7 times, most recently from 1a6c9d9 to 9a2df3a Compare

March 3, 2017 14:49

Collaborator Author

ChadKillingsworth commented Mar 3, 2017

@brad4d I took a hard run through and reworked this PR. I was able to drastically simplify things and hopefully the entire file is now much easier to read and follow.

brad4d requested changes

View reviewed changes

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

@@ @@ -232,6 +248,14 @@ public ModulePath resolveJsModule( @@
                   @Nullable
                   private String resolveJsModuleFile(String moduleAddress) {
+                    String[] fileExtensionsToSearch;

Contributor

brad4d Mar 8, 2017

How about passing the file extensions to search into this method instead?
I'd prefer to keep checking of ResolutionMode to only the one switch statement, even if it means creating some custom-named functions for particular code paths.
Such functions make it easier to know the context when you're reading.

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

                       if (loadAddress != null) {
-                        return loadAddress;
+                        return removeAbsoluteIdentifierIndicator(loadAddress);

Contributor

brad4d Mar 8, 2017

Could you add a comment to say why you remove the absolute identifier indicator here?
Maybe in javadoc on the method.

Collaborator Author

ChadKillingsworth Mar 8, 2017

Apparently there wasn't a good reason ...

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

-                    if (!modulePaths.contains(resolved) && errorHandler != null) {
-                      errorHandler.report(
-                          CheckLevel.WARNING, JSError.make(sourcename, lineno, colno, LOAD_WARNING, moduleName));
+                    String resolved = resolveJsModuleFile(moduleName); //locateNoCheck(moduleName);

Contributor

brad4d Mar 8, 2017

should probably remove the //locateNoCheck comment.

ChadKillingsworth force-pushed the js-module-root branch 2 times, most recently from 5235847 to d44dd2b Compare

March 8, 2017 03:50


          Fix the interaction between absolute path imports an module roots.

96e5df8

ChadKillingsworth force-pushed the js-module-root branch from d44dd2b to 96e5df8 Compare

March 8, 2017 03:51

Collaborator Author

ChadKillingsworth commented Mar 8, 2017

@brad4d Fixed.

brad4d approved these changes

View reviewed changes

brad4d requested changes

View reviewed changes

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

@@ @@ -388,38 +417,66 @@ public static boolean isPathIdentifier(String name) { @@
                   return name.contains(MODULE_SLASH);
                 }
+                /** Removes the leading slash from absolute identifiers */
+                public static String removeAbsoluteIdentifierIndicator(String name) {

Contributor

brad4d Mar 9, 2017

This isn't called anywhere.

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

+                }
+                /** Converts paths which don't start with either a '.' or '/' to absolute paths */
+                public static String toAbsoluteIdentifier(String name) {

Contributor

brad4d Mar 9, 2017

Why is this public?

src/com/google/javascript/jscomp/deps/ModuleLoader.java Outdated

+                /** Converts paths which don't start with either a '.' or '/' to absolute paths */
+                public static String toAbsoluteIdentifier(String name) {
+                  if (name.length() == 0 || isAbsoluteIdentifier(name) || isRelativeIdentifier(name)) {
+                    return name;

Contributor

brad4d Mar 9, 2017

Wait, this can return a relative identifier?
That's just misleading.
I saw your post regarding troublesome terminology, though I cannot find it now.
I propose this.

"absolute" means "starts with '/'"
"relative" means "starts with './'"
"ambiguous" means everything else.

How about we detect ambiguous paths ASAP and either report an error or explicitly force them to be relative or absolute at that point?

I'd rather see some duplicated logic that makes it clear what is happening than a "fix it if it is broken, otherwise leave it alone" method like this one.

Collaborator Author

ChadKillingsworth Mar 9, 2017

How about we detect ambiguous paths ASAP and either report an error or explicitly force them to be relative or absolute at that point?

I think ambiguous paths (as you labeled them) are used widely inside google. For instance, this path is ambiguous:

import {foo} from 'path/to/bar'

I can force them to be absolute if you are willing to try landing it ...

Collaborator Author

ChadKillingsworth Mar 9, 2017

FYI that path means two completely different things depending on the resolution mode. This has been a source of great confusion to all parties.

import {foo} from 'path/to/bar'

In LEGACY mode, this path is absolute.
In NODE mode, this path is looked up from the registry.
In BROWSER mode, this path is invalid.

Contributor

brad4d commented Mar 9, 2017

@ChadKillingsworth
Our "squash to a single commit" policy doesn't seem to work well with code reviews.
Since we're having some back and forth here, I suggest you not do the squashing until we've come to agreement.

Collaborator Author

ChadKillingsworth commented Mar 23, 2017

@brad4d I reworked the resolution algorithms into independent classes.

ChadKillingsworth force-pushed the js-module-root branch from 54b1353 to b6767f9 Compare

March 23, 2017 13:43


          Refactor module resolution algorithms into separate classes

c175780

ChadKillingsworth force-pushed the js-module-root branch from b6767f9 to c175780 Compare

March 23, 2017 13:48

ChadKillingsworth mentioned this pull request

Node module resolution does not normalize paths with trailing slashes #2377

Closed

brad4d approved these changes

View reviewed changes

Contributor

brad4d commented Mar 30, 2017

Looking very good.
Thanks!
Importing and testing.

Contributor

brad4d commented Apr 2, 2017

Had to fix some internal stuff to know about the new files.
All our project tests passed.
I'm testing against all internal projects now.

Contributor

brad4d commented Apr 2, 2017

Got a couple-dozen failures when testing all the things.
I'm part way through confirming which are "real" breaks and which were actually already broken.

Collaborator Author

ChadKillingsworth commented Apr 3, 2017

I would actually expect a few project fixes to be required by this change - but it is definitely worth the trouble.

Contributor

brad4d commented Apr 7, 2017

None of the breaks appear to be real.
Sent for internal review.

brad4d added 3 commits

April 7, 2017 12:46


          add javadoc comment

22f1bfb


          ran git5 fix

15728b8


          Import of external changes in review client.

b21e179

Contributor

MatrixFrog commented Apr 7, 2017

Just to be clear, you don't have to push changes like formatting changes back to the Github PR for MOE to work.

brad4d requested changes

View reviewed changes

src/com/google/javascript/jscomp/deps/BrowserModuleResolver.java Outdated

+                    String scriptAddress, String moduleAddress, String sourcename, int lineno, int colno) {
+                  if (ModuleLoader.isAmbiguousIdentifier(moduleAddress)) {
+                    if (errorHandler != null) {

Contributor

brad4d Apr 7, 2017

Could we forbid null for errorHandler and just make the creator pass in a no-op Errorhandler if they don't care about errors?

src/com/google/javascript/jscomp/deps/ModuleResolver.java Outdated

+                }
+                Map<String, String> getPackageJsonMainEntries() {
+                  return null;

Contributor

brad4d Apr 7, 2017

Could we return an empty map instead of null so this method doesn't have to be @Nullable?

Contributor

brad4d commented Apr 7, 2017

@ChadKillingsworth
I've pushed some tweaks from my internal review with @MatrixFrog onto your branch.

I've also passed along 2 requests for changes in a github code review.

Regarding testing we are looking great.
All of the global presubmit tests passed.
As long as we make no further behavior changes on this PR request, we should be able to submit with just our standard project tests, which complete very quickly.

Contributor

brad4d commented Apr 7, 2017

@MatrixFrog
Yeah, I know I didn't have to push the changes back, but I'm trying to make the review process more transparent, and I've worked out a way to do it that's pretty easy and will get easier as I tweak it further.

Collaborator Author

ChadKillingsworth commented Apr 8, 2017

@brad4d So the 64million dollar question... Do I need to squash my commits when I push next?


          Add a noop error handler so that the error handler does not need to b…

59bde15

…e nullable. Returns an empty map from getPackageJsonMainEntries so it does not return null.

Contributor

brad4d commented Apr 8, 2017

@ChadKillingsworth
Nope, no need for you to squash at all.
I'll effectively squash it on my end.

In case you're interested, I'm basically working like this.

cd $my_github_clone
git pull $chads_pr_branch
git format-patch -stdout >patchfile
patch-munge <patchfile >patchfile.munge # secret sauce to fix paths, etc.
cd $my_internal_repo_clone
git am patchfile.munge

Then the reverse to go back out to you.
This has no effect on what happens when I submit internally and the result is pushed to our github repo.

Collaborator Author

ChadKillingsworth commented Apr 8, 2017

Awesome. I made the requested changes. Should be good-to-go.


          Remove another unneeded null check since the errorhandler cannot be n…

0ac50c3

…ull.

brad4d approved these changes

View reviewed changes

Contributor

brad4d commented Apr 11, 2017

I've imported your latest change and hit "the submit button".
With luck this will be in tomorrow's push to github.

This was referenced Apr 11, 2017

Allow comments in flagfile #2413

Open

Reduce --js_module_root verbosity #2423

Closed

brad4d closed this in

e51b5bd

ChadKillingsworth deleted the js-module-root branch

April 11, 2017 19:48

ChadKillingsworth mentioned this pull request

April 2017 open source release #2456

Closed

Yannic pushed a commit to Yannic/com_google_closure_compiler that referenced this pull request


          Fix the interaction between absolute path imports and module roots

1a2c558

Closes google#2343

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=152771274

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels