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

Circular dependencies unresolved in Meteor 1.5 #9176

Closed
GeoffreyBooth opened this Issue Oct 5, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@GeoffreyBooth
Contributor

GeoffreyBooth commented Oct 5, 2017

Circular dependencies are the bane of my existence.

So I took the Todos app coffeescript branch and first updated it to be current with master, which worked fine (not much has changed since the last time I did so). master is still on Meteor 1.4.

Then I updated to the coffeescript branch to Meteor 1.5.2.2 and coffeescript@1.12.7_1. I wasn’t expecting this to cause problems, but it did. When you run Todos with that combo, the circular dependency issue rears its head again. You can reproduce it by checking out this branch, running meteor, and adding a todo item to a list. You’ll see an alert saying undefined. The imported symbol Todos is undefined where it’s used. Set a breakpoint in incompleteCountDenormalizer.coffee:11, the incompleteCount = Todos.find line; Todos is undefined, though it shouldn’t be.

This is befuddling, since I’m using import and export statements that are compiled by Babel, so if the modules get resolved correctly on master they should resolve likewise here. I forked the coffeescript packages to ensure that they’re using exactly the same versions of ecmascript and Babel as master, but I still got the error. Yet the master branch on 1.5.2.2 with the same package versions runs fine. Why would the coffeescript branch behave any differently?

The alert error is thrown because Todos (and Lists) are undefined in incompleteCountDenormalizer.coffee. I went down to the lowest level after-Babel, running-in-browser code, and both todos.coffee and incompleteCountDenormalizer.coffee are output nearly identically. The module export code for todos is the same for both:

module.export({
  Todos: function() {
    return Todos;
  }
});
//
var Todos = new TodosCollection("todos");

as is the import:

var Todos = void 0;
module.watch(
  require("./todos.js"), // or require("./todos.coffee"),
  {
    Todos: function(v) {
      Todos = v;
    }
  },
  2
);

I’m flummoxed. https://github.com/GeoffreyBooth/todos-coffeescript/tree/circular

Possibly related to #7459.

@klaussner

This comment has been minimized.

Collaborator

klaussner commented Oct 7, 2017

The difference between the CoffeeScript and JavaScript versions is that for each source.coffee file, an additional source.coffee.js file is generated. This file contains the compiled CoffeeScript code, whereas the source.coffee file looks like this:

module.exports = require('source.coffee.js');

It just re-exports everything from source.coffee.js. The problem with this CommonJS-style re-export is that it doesn't work for circular dependencies. Take this reproduction (repo here), for example:

// foo.js
import { bar } from './bar.js';

export function foo() {
  bar();
}

// foo.re-export.js
module.exports = require('./foo.js');

// bar.js
import { foo } from './foo.re-export.js';

export function bar() {
  console.log(!!foo);
}

// index.js
import { foo } from '/imports/foo.re-export.js';
foo();

The index.js file imports foo.re-export.js, which re-exports foo.js; foo.js and bar.js have a circular dependency via foo.re-export.js. This is what happens when index.js is evaluated:

  1. foo.re-export.js is imported and requires foo.js.
  2. foo.js imports bar.js.
  3. bar.js imports foo.re-export.js but foo is undefined at this point because foo.re-export.js is already being evaluated and its module.exports object is empty.
  4. bar is exported.
  5. Evaluation of foo.js continues with bar, the function exported from bar.js.
  6. foo is exported.
  7. bar.js doesn't get the new value of foo because foo.re-export.js has now been evaluated but the module.watch setter in the generated code of bar.js isn't called again. ⚡️
  8. Evaluation of index.js continues with foo, the function exported from foo.js.
  9. foo is called.
  10. bar is called.
  11. foo in bar.js is still undefined, so the output is false.

If you import foo.js (instead of foo.re-export.js) in bar.js, the module will get the new value of foo from step six via module.watch and the bar function will output true in the console.

@klaussner

This comment has been minimized.

Collaborator

klaussner commented Oct 7, 2017

It looks like Meteor generates the re-export automatically if an input file (source.coffee) has a different file name than its output file (source.coffee.js). If I remove the .js extension in this line in coffeescript-compiler, the re-exports are not generated and circular dependencies work as expected:

return inputFile.getPathInPackage() + '.js';

GeoffreyBooth added a commit to GeoffreyBooth/meteor that referenced this issue Oct 7, 2017

Keep the same outputFilePath as the inputFilePath was, to prevent Met…
…eor from adding a second layer of exports, to prevent a circular dependency; see meteor#9176 (comment)
@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Oct 8, 2017

@klaussner Thank you so much for getting to the bottom of this! So I went back to my circular branch and rewound to the commit where I had just updated to Meteor 1.5, but not yet updated the coffeescript package. This issue was still present in Meteor 1.5.2.2 with coffeescript@1.11.1_4. So I guess something changed with module loading in Meteor 1.5.

The line you cite was added in f7ee0ba (by @glasser, in 2015) and not changed meaningfully since then. The coffeescript package was at version 1.0.7 back then. Though maybe the later addition of Babel compilation affected this.

Going back to my circular branch, I copied the current coffeescript and coffeescript-compiler packages (1.12.7_1) from meteor/non-core/packages into todos/packages. Then I bumped the versions of the local versions to 1.12.7_2 and removed the + '.js'. Lo and behold, that solved the issue. I created #9190 with this fix.

So this begs the question: is there any harm in removing that + '.js'? The tests still pass and source maps still work.

@benjamn benjamn added this to the Package Patches milestone Oct 8, 2017

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Oct 9, 2017

On the other hand, should Meteor be generating these “re-export” files

module.exports = require('./foo.js');

? Rather than patching the coffeescript package to use the same filename to avoid these files getting auto-generated, should we instead patch the code that generates these “re-export” files to not do so?

@klaussner

This comment has been minimized.

Collaborator

klaussner commented Oct 9, 2017

It looks like the .js extension is added to the output filename since the very first version of the coffeescript package, so maybe it's just a leftover from older versions of the build system. 👵 The only difference that I've noticed after removing it is that the .coffee.js files with the compiled CoffeeScript code no longer show up in the DevTools under "Sources" (I don't know if that was intentional in the first place).

But I agree that it makes sense to look into why the re-exports are generated that way because other build plugins could run into this issue as well. The function that generates the code is _checkSourceAndTargetPaths in /tools/isobuild/import-scanner.js:

// Make sure file.sourcePath is defined, and handle the possibility that
// file.targetPath differs from file.sourcePath.
_checkSourceAndTargetPaths(file) {
file.sourcePath = this._getSourcePath(file);
if (! isString(file.targetPath)) {
return;
}
file.targetPath = pathNormalize(pathJoin(".", file.targetPath));
if (file.targetPath !== file.sourcePath) {
const absSourcePath = pathJoin(this.sourceRoot, file.sourcePath);
const absTargetPath = pathJoin(this.sourceRoot, file.targetPath);
// If file.targetPath differs from file.sourcePath, generate a new
// file object with that .sourcePath that imports the original file.
// This allows either the .sourcePath or the .targetPath to be used
// when importing the original file, and also allows multiple files
// to have the same .sourcePath but different .targetPaths.
let sourceFile = this._getFile(absSourcePath);
if (! sourceFile) {
const installPath = this._getInstallPath(absSourcePath);
sourceFile = this._addFile(absSourcePath, {
type: file.type,
sourcePath: file.sourcePath,
servePath: installPath,
installPath,
dataString: "",
deps: {},
lazy: true,
});
}
// Make sure the original file gets installed at the target path
// instead of the source path.
file.installPath = this._getInstallPath(absTargetPath);
file.sourcePath = file.targetPath;
const relativeId = this._getRelativeImportId(
absSourcePath,
absTargetPath,
);
// Set the contents of the source module to import the target
// module(s). Note that module.exports will be set to the exports of
// the last target module. This is not perfect, but (1) it's better
// than trying to merge exports, (2) it does the right thing when
// there's only one target module, (3) the plugin author can easily
// control which file comes last, and (4) it's always possible to
// import the target modules individually.
sourceFile.dataString += "module.exports = require(" +
JSON.stringify(relativeId) + ");\n";
sourceFile.data = new Buffer(sourceFile.dataString, "utf8");
sourceFile.hash = sha1(sourceFile.data);
sourceFile.deps[relativeId] = {};
}
}

@glasser

This comment has been minimized.

Member

glasser commented Oct 9, 2017

I may have moved that line around but that entire project ("batch compiler plugins") predates even thinking about the idea of native ES6 in Meteor, let alone ES6 modules, so I wouldn't read too much into anything from that era or earlier.

GeoffreyBooth added a commit to GeoffreyBooth/meteor that referenced this issue Oct 10, 2017

Keep the same outputFilePath as the inputFilePath was, to prevent Met…
…eor from adding a second layer of exports, to prevent a circular dependency; see meteor#9176 (comment)
@benjamn

This comment has been minimized.

Member

benjamn commented Oct 11, 2017

I agree that #9190 is the best solution to this problem, but I wanted to explain a bit about the motivation for the re-exporting.

A compiler plugin can call inputFile.addJavaScript more than once for the same inputFile, with different output path, different code, different laziness, etc. This produces multiple output modules for the same source file. The output modules can be imported individually, using their specific paths, but the more common pattern is to import the original source file (that is, the .coffee module rather than the .coffee.js module). This raises the question of what should happen when you import a source file that has multiple output files.

My answer in the Meteor 1.3 days was that importing the source module should have the effect of importing and re-exporting all of the output modules. Usually there's only one output module, so this ends up looking like module.exports = require("./output-module.coffee.js");, but you could end up with a series of imports in the source module, if there are multiple output modules.

Please believe me when I say I never wanted to support the multiple output modules use case, but was forced to do so by existing compiler plugins back in the Meteor 1.3 days.

One big problem with the module.exports = require(id) idiom (in general, not just in Meteor) is that it interferes with the way CommonJS handles circular dependencies, because it replaces the original module.exports object, which might have been observed by another module involved in the cycle at some earlier point. Hence the problems @GeoffreyBooth and @klaussner encountered while debugging this issue.

In order to preserve the original module.exports object, another approach we might try is to copy the exports of the output module(s) over to the exports object of the source module:

Object.assign(exports, require("./output-1.coffee.js"));
Object.assign(exports, require("./output-2.coffee.js"));
Object.assign(exports, require("./output-3.coffee.js"));

However, this only copies the exports once, and never updates them again, so it can also cause problems if you have circular dependencies, and one of the output modules hasn't finished evaluating yet.

Fortunately, because we have to support export * from "./module" anyway, we now have another much better way to re-export modules without replacing module.exports, with the exports also remaining up-to-date, so that circular dependencies work as well as possible:

module.watch(require("./output-1.coffee.js"), {
  "*": function (ns) {
    Object.assign(module.exports, ns);
  }
});

Because this is a common idiom in Reify-generated code, there's a shorthand for it:

module.watch(require("./output-1.coffee.js"), { "*": module.makeNsSetter() });
module.watch(require("./output-2.coffee.js"), { "*": module.makeNsSetter() });
module.watch(require("./output-3.coffee.js"), { "*": module.makeNsSetter() });

With module.watch, whenever any of the output modules exports something new, it will get copied over to the module.exports object of the source module.

I'll be submitting a PR soon to use module.watch instead of reassigning module.exports, which should fix the underlying problem. However, #9190 is still a worthwhile solution, because it means we won't have to generate these unnecessary wrapper modules at all.

benjamn added a commit that referenced this issue Oct 11, 2017

Use module.watch live bindings to solve #9176.
Further explanation / discussion:
#9176 (comment)

Another (complementary) solution to the same problem: #9190

benjamn added a commit that referenced this issue Oct 12, 2017

Use module.watch live bindings to solve #9176.
Further explanation / discussion:
#9176 (comment)

Another (complementary) solution to the same problem: #9190

benjamn added a commit that referenced this issue Oct 12, 2017

Keep the same outputFilePath as the inputFilePath was, to prevent Met…
…eor from adding a second layer of exports, to prevent a circular dependency; see #9176 (comment)
@benjamn

This comment has been minimized.

Member

benjamn commented Oct 13, 2017

I believe this is fixed now, thanks to #9190. Thanks again @GeoffreyBooth!

@benjamn benjamn closed this Oct 13, 2017

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Oct 13, 2017

Unless you think a test should be added for it?

benjamn added a commit that referenced this issue Oct 31, 2017

Use module.watch live bindings to solve #9176.
Further explanation / discussion:
#9176 (comment)

Another (complementary) solution to the same problem: #9190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment