vazco / universe-modules

Use ES6 / ES2015 modules in Meteor with SystemJS
https://atmospherejs.com/universe/modules
MIT License
52 stars 7 forks source link

Bad interaction between modules, local-test, and package namespaces #5

Closed tel closed 9 years ago

tel commented 9 years ago

Module loader fails repeatedly with odd messages when testing a package that exports namespace-bound modules.

Potentially unhandled rejection [3] TypeError: Failed to construct 'URL': Invalid URL
    at TypeError (native)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:2708:32)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4266:34)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4277:44)
    at SystemJSLoader.normalizeSync (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4410:48)
    at SystemJSLoader.normalizePlugin (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4568:38)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4575:44)
    at SystemJSLoader.normalize (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:5058:54)
    at SystemJSLoader.esmRegEx (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:3502:71)
    at SystemJSLoader.loader (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:3703:36)

Steps to replicate

  1. Create a new Meteor project
  2. Create a package within it at a given namespace, e.g. meteor create --package ns:pkg
  3. Add universe:modules as a dep for both onUse and onTest
  4. Create a normal, exported module and add the file, e.g. x.import.js
  5. Write a test suite like the following and add it to the onTest section
System.import("ns:pkg/x").then(function (x) {
  Tinytest.add("Ex", function (test) { test.equal(1, 1) });
});
  1. Run this test locally with meteor test-packages ns:pkg
  2. Observe that errors occur in the server log, the browser console, and that no client-side tests run

Then, optionally, remove the ns: namespace and observe it all work fine.

tel commented 9 years ago

Possible reason for failure:

When running tests locally Meteor prepends another segment to the base name of the package. In this case, ns:pkg becomes local-test:ns:pkg in the meteor test-packages environment. The double-colon syntax may wreck havoc on System.js' loader.

MacRusher commented 9 years ago

Thank you for really good reproduction steps.

Did you add api.addFiles('x.import.js') inside onUse?

It's working when I add it. The reason is that for module to be available it need to be parsed by meteor build plugin. Inside app every file is parsed, but in packages files need to be added explicit, and universe:modules is not changing this behavior.

Let me know if this was the issue.

When running tests locally Meteor prepends another segment to the base name of the package. In this case, ns:pkg becomes local-test:ns:pkg

You're sure about that? Inside build plugin, compileStep.packageName is still ns:pkg

tel commented 9 years ago

Here's a more direct failing case (non-minimal, though). The package file looks like

Package.describe({
  name: 't:t',
  version: '0.0.1',
  // Brief, one-line summary of the package.
  summary: '',
  // URL to the Git repository containing the source code for this package.
  git: '',
  // By default, Meteor will default to using README.md for documentation.
  // To avoid submitting documentation, set this field to null.
  documentation: 'README.md'
});

Package.onUse(function(api) {
  api.versionsFrom('1.1.0.2');
  api.use("universe:modules");
  api.addFiles('t.import.js');
  api.addFiles('a/t.import.js');
  api.addFiles('a/a/t.import.js');
  api.addFiles('a/a/a/t.import.js');
});

Package.onTest(function(api) {
  api.use('tinytest');
  api.use("universe:modules");
  api.use('t:t');
  api.addFiles('tests.import.js');
  api.addFiles('t-tests.js');
});

and here's t-tests.js

// System.import("./tests");
System.import("t:t/t").then(function(t) {
  Tinytest.add("Example", function(test) {
    test.equal(t.a, 3);
  });
});

The way that I'm aware of the local-test bit is by looking at the compiled artifacts themselves to see the paths. It also shows up (sometimes, not entirely clear when) in System.defined on the client.

MacRusher commented 9 years ago

I've created PR fixing your issue: https://github.com/tel/meteor-universe-modules-failing-example/pull/1

When importing from a package, you need to add full name, in your case t:t/t and not just t/t.

Tests are also passing. Let me know if it works for you. If you could break it again I will try to fix it again ;-)

Default SystemJS error reporting is pretty missleading in context of Meteor, but we will improve this in future versions of universe:modules.

tel commented 9 years ago

I'll check that in a minute. I agree that what you've addressed is an error, but I would have thought only in the main application, not meteor test-packages.

tel commented 9 years ago

So, this does not appear to fix the issue:

image

While "all tests pass" it's not even registering the test on the client.

In case it's relevant, I've tested this behavior on Chrome and Safari.

For the time being in my own projects I'm just going to eliminate all of my "vendor" prefixes. That appears to eliminate the issue—you can test it out in this repo by changing the name of package t:t to the unqualified t.

MacRusher commented 9 years ago

Hmm strange, it works on chrome@windows, but just tested on firefox@linux and I have the same issue. Give me a moment I will investigate this.

tel commented 9 years ago

I'm trying a branch where I've eliminated all of my company-specific namespace prefixes... and the issue persists!

> System.import("local-test:sums/sums-tests")
Promise {_handler: Pending}
system-polyfills.js:210 Potentially unhandled rejection [15] TypeError: Failed to construct 'URL': Invalid URL
    at TypeError (native)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:2708:32)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4266:34)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4277:44)
    at SystemJSLoader.normalizeSync (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4410:48)
    at SystemJSLoader.normalizePlugin (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4568:38)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4575:44)
    at SystemJSLoader.normalize (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:5058:54)
    at SystemJSLoader.esmRegEx (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:3502:71)
    at SystemJSLoader.loader (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:3703:36)(anonymous function) @ system-polyfills.js:210report @ system-polyfills.js:237flush @ system-polyfills.js:259

> System.import("immutable")
Promise {_handler: Pending}
tel commented 9 years ago

To clear up the prior log: it feels like there's a URL mapping issue arising when something is prefixed by local-test:. On the other hand

System.defined
Object {http://localhost:3000/immutable/index: undefined, http://localhost:3000/immutable/proptypes: Object, http://localhost:3000/sums/module: Object, http://localhost:3000/sums/either: Object, http://localhost:3000/sums/maybe: Object…}

http://localhost:3000/immutable/index: undefined
http://localhost:3000/immutable/proptypes: Object
http://localhost:3000/sums/either: Object
http://localhost:3000/sums/index: Object
http://localhost:3000/sums/maybe: Object
http://localhost:3000/sums/module: Object
local-test:sums/describe: Object
local-test:sums/either-tests: Object
local-test:sums/maybe-tests: Object
local-test:sums/sums-tests: Object__proto__: Object

This demonstrates that to some degree or another the local-test namespace was properly registered.

tel commented 9 years ago

Interesting that it's client specific. I think it's coming down to a bug in the polyfill. The request that it's making gets rejected in some but not all environments. I wish I knew how to dig deeper and figure out what those failing URLs look like.

tel commented 9 years ago

Walking through with the debugger lands (in real code) on a failure here

> System.normalizeSync("./describe", "local-test:sums/sums-tests")
Uncaught TypeError: Failed to construct 'URL': Invalid URL
    at TypeError (native)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:2708:32)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4266:34)
    at SystemJSLoader.<anonymous> (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4277:44)
    at SystemJSLoader.normalizeSync (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4410:48)
    at SystemJSLoader.normalizePlugin (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4568:38)
    at SystemJSLoader.loader [as normalizeSync] (http://localhost:3000/packages/universe_modules.js?664026707e7e9b793cd58753869fc2f60e3f3c52:4581:44)
    at <anonymous>:2:8
    at Object.InjectedScript._evaluateOn (<anonymous>:895:140)
    at Object.InjectedScript._evaluateAndWrap (<anonymous>:828:34)

where describe.import.js is a module used only in the onTest segment of the app.

tel commented 9 years ago

Inlining that import fixes the error.

tel commented 9 years ago

So does changing it to an absolute path:

import describe from "./describe";                 // fails, "Invalid URL"
import describe from "describe";                   // fails, "Uncaught SyntaxError: Unexpected token <"
import describe from "local-test:sums/describe";   // works
MacRusher commented 9 years ago

I think I know where is the error I just don't know yet why.

Protip: you can get nice error if you catch promise rejection.

System.import("k").then(function (k) {
    console.log('k', k)
}, function (err) {
    console.warn(err);
});

You should get something like:

Error: ./a/t is not a valid URL.
    Error loading t:t/t from http://localhost:3000/k
    Error loading t:t/t

I'm writing custom loader right now so we can have more control over SystemJS actions.

MacRusher commented 9 years ago

Is seems that there are some differences in System.normalize implementation between clients. On chrome@windows it receives file:///T:/t/t as a parent and it's working, but on firefox@ubuntu parent is t:t/t. I hope that once I finish loader that is more Meteor-specific this problem should go away.

MacRusher commented 9 years ago

Ok, I've refactored the package a bit, version 0.3.0 should work now.

The problem was with browser URL object implementation. Now polyfill is used as default so it will work the same on all browsers.

Thanks for really interesting use case! Let me know if something still isn't working for you.

tel commented 9 years ago

That's beautiful! Thanks for pushing it through. Things seem to be working now... with one minor regression.

tel commented 9 years ago

See #9