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

Loading pure typescript packages #720

Closed
nicklasb opened this issue Aug 23, 2015 · 39 comments
Closed

Loading pure typescript packages #720

nicklasb opened this issue Aug 23, 2015 · 39 comments

Comments

@nicklasb
Copy link

Hi,
I am writing a package that I want to keep in pure TypeScript.
The problem is that while it works well to load TypeScript files locally using package and defaultExtension, it becomes a different matter when I create a package and then install it using jspm.

Because then it feels like I lose control, specifying package extensions like this:

  packages: {
     "jspm_packages/github/OptimalBPM/mbe-nodes*": {
        "defaultExtension": "ts"
      }
}

And "jspm_packages/github/OptimalBPM/mbe-nodes@0.4.9" or any other combinations I have tried doesn't help. It keeps trying to load the .ts.js either way.

NOTE: I am working against a local registry.

@guybedford
Copy link
Member

Yes, unfortunately these types of package configurations don't currently share on installing dependencies, and does require manually copying the package config in. This is being worked on for jspm 0.17 to support these properties in the package.json.

@nicklasb
Copy link
Author

Good stuff, tell me when you have a beta with that in it if you want some testing done or if there is something else I can do.

@nicklasb
Copy link
Author

BTW, just to work around until then (and see that there is no other problem with my setup), what should be copied where? Something into .jspm.json or what?

@guybedford
Copy link
Member

Try using a configuration like -

packages: {
     "github:OptimalBPM/mbe-nodes@x.y.z": {
        "defaultExtension": "ts"
      }
}

@nicklasb
Copy link
Author

I have tried that, I have used an index.js, index.ts, rewritten mbe-nodes@0.4.9 and all kinds of stuff, and still it persists in trying to get the .ts.js if I write nodes.ts, .js if I write nodes.

It is like that isn't happening in the same context as the rest of the loading, as if it doesn't care about defaultExtension for external libraries.

@guybedford
Copy link
Member

Ah, ok I guess the package config is being overridden by the map config here. Try adding it to a separate System.config call that then also adds the original map back -

System.config({
     "github:OptimalBPM/mbe-nodes@x.y.z": {
        "defaultExtension": "ts",
        "map": {...whatever is in the config.js map for this package if anything...}
      }
});

We should probably ensure that a package config always composes instead of replacing.

@guybedford
Copy link
Member

Hmm actually package configs do already compose so that shouldn't be the issue. The way to debug this is to check that System.packages contains the configuration you expect under the full URL to the package.

@nicklasb
Copy link
Author

No, it didn't work, I'll do that and get back to you.

@nicklasb
Copy link
Author

It seems ok. And in some way it is as you say, why wouldn't it? It composes?
image

To me, it rather seems like it doesn't care at all about that configuration when it loads external libraries.

@nicklasb
Copy link
Author

I am trying to wrap my head around the code, and find out where what happens, but there is lots of it.

It struck me that SystemJS is exactly the kind of application that would benefit greatly from being converted into TypeScript, for structure and readability. Just had to say it. Not that you wanted to hear it. :-)

@guybedford
Copy link
Member

Are the paths loaded as .js definitely paths within /optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.9/... here? Also are you running the very latest release of SystemJS?

@nicklasb
Copy link
Author

I think I have found it.

I missed that the 404 is not for OptimalBPM/mbe-nodes, but for:
"http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/dist/nodes.ts.js".
So it looks at "OptimalBPM/dist/nodes.ts.js", not "OptimalBPM/mbe-nodes/dist/nodes.ts.js".

The reason is that the index.js-file in the mbe-nodes package seems to be parsed relative to the parent folder, not the package folder, so this doesn't work:
module.exports = require("./dist/nodes.ts");

But this does:
module.exports = require("./mbe-nodes@0.4.9/dist/nodes.ts");

However, the second variant is really cumbersome, as that means one more place to update the version information in the package.

Interestingly, removing the "./":
module.exports = require("dist/nodes.ts");
...makes it look in the root of the application: "http://localhost:63342/optimalbpm/optimalbpm/broker/ui/dist/nodes.ts.js".

I used an index.js-file, as that seemed to be the only way to load it properly.

@guybedford
Copy link
Member

Are you saying that the file at OptimalBPM/mbe-nodes/index.js contains a require to require('./dist/nodes.ts') which is resolving to OptimalBPM/dist/nodes.ts.js? If so that isn't the correct resolution at all. Relative modules are always module-relative. This normalization can also be checked via System.normalizeSync('./dist/nodes.ts', 'http://http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@x.y.z/index.js') which should give what you expect here. Let me know if you're able to track that down further.

@nicklasb
Copy link
Author

Yes, that is what I am saying.

Running that in the index.html script tag yields:
http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10/dist/nodes.ts
So that seems to work. However that is running in the index.html, not sure if there is some contextual issues I don't get. I'll see what I can find out.

@nicklasb
Copy link
Author

Not sure if that is relevant, but running it without the "./" returns
"http://localhost:63342/optimalbpm/optimalbpm/broker/ui/dist/nodes.ts.js", which it also does when I remove "./" the index.js, suggesting that it operates from the same "current directory", it that is applicable.

@guybedford
Copy link
Member

Yes - that would be the baseURL then. Are you sure there isn't another require to dist/nodes.ts somewhere else in the codebase (without the ./)?

@nicklasb
Copy link
Author

I searched through the entire project "dist/nodes", and outside gulpfile.js, of course, I only find TypeScript imports, but even if they are transpiled into requires, they are inside the package.

And as per above, it starts working if I change index.js into:
module.exports = require("./mbe-nodes@0.4.9/dist/nodes.ts");

So it would appear that the error is actually from there.

@guybedford
Copy link
Member

That is very strange. Is there anyway at all you can share this test case for me to take a look?

@nicklasb
Copy link
Author

It kind of sucks, but the code is not in a public repo yet. I will see if I can provide some example.

@nicklasb
Copy link
Author

Is the "normalize" hook the normalizeSync-implementation, and what is used to find the correct path throughout?

@guybedford
Copy link
Member

System.normalize(name, parentURL).then(...) is the normalize function used internally.

@nicklasb
Copy link
Author

This is what fails:
new URL(name, parentName || baseURIObj).href;
given these parameters:

..returns:
http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/dist/nodes.ts

@guybedford
Copy link
Member

That normalization is actually correct. Are you saying the file at http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10.js contains require('./dist/nodes.ts') or are you using custom package configuration here?

@nicklasb
Copy link
Author

Btw it is here it is being called:

return new URL(name, parentName || baseURIObj).href;

No, it is the http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10/index.js that contains the following:

/* */ 
"format cjs";
module.exports = require("./dist/nodes.ts");

@nicklasb
Copy link
Author

If I change that to:

/* */ 
"format cjs";
module.exports = require("./mbe-nodes@0.4.10/dist/nodes.ts");

...it works.

@guybedford
Copy link
Member

parentName inside of http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10/index.js should most certainly include the index.js part. Does module.id give the correct normalized name?

@nicklasb
Copy link
Author

I don't understand what you mean now..

@guybedford
Copy link
Member

The issue as you've shown is that the parentName being normalized to is being seen as http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10 instead of http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10/index.js resulting in a lower normalization. Something must be causing the parent to be seen as http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10 in the first place though, and that I can't guess without some way to replicate the scenario.

@nicklasb
Copy link
Author

You are right.
if I, while breaking, adds index.js like this:
new URL(name, parentName +"/index.js").href

it returns the correct:
http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10/dist/nodes.ts

@nicklasb
Copy link
Author

I'll debug some, I'll be back.

@nicklasb
Copy link
Author

Ok. there is a difference between the other packages.
In instantiate, the load.name for mbe-nodes is:
"http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10"
The ".js" in the end is missing(or "index.js", for that matter).

Could this be related to defaultExtension being ".ts" or something like that?

@nicklasb
Copy link
Author

It is. If I remove the defaultExtension: "js" for the package, it gets the proper load.name.

@guybedford
Copy link
Member

That is great to hear you've found a change that fixes it. In order to work out the issue though I do still need a full description of the issue though. Here's what I've got so far:

You have a package that you installed via jspm, that is CommonJS, so the folder structure is:

jspm_packages/github/package/name@x.y.z.js
module.exports = require('github:package/name@x.y.z/index.js');

jspm_packages/github/package/name@x.y.z/index.js
module.exports = require('./dist/nodes.ts');

jspm_packages/github/package/name@x.y.z/dist/nodes.ts
the file you want loaded

As well as this you've then added custom SystemJS configuration:

System.config({
  packages: {
    'github:package/name@x.y.z': {
      defaultExtension: 'js'
    }
  }
});

To summarize the issue:

  • With the above you're getting an incorrect module load to github:package/dist/nodes.ts when running `System.import('package/name')
  • If you remove the defaultExtension custom config it then works.

Does the above sound like it covers the full scenario? Are you able to reduce your test case to something similar, or do you think there are other factors at work here at all?

@nicklasb
Copy link
Author

Yes, except from some nitpicks:

The jspm_packages/github/package/name@x.y.z.js is
module.exports = require('github:package/name@x.y.z/index');, not index.js, but that doesn't affect the result.

I am not running a System.Import, just a typescript import "mbe-nodes" (which I presume is transpiled into a ES6 import) in my main.ts(that initiates the application). But I suspect that should be the same.

Yes, If I remove defaultExtension it loads github:package/name@x.y.z, the correct path is used, of course then the package doesn't work beyond that, but that is another matter.

(I realize that I perhaps should rename nodes.ts to mbe-nodes.ts, but that is another matter)

@guybedford
Copy link
Member

Thanks so much for your help tracking this down, I've added a possible fix in b7abc2c. Try running jspm dl-loader --edge to upgrade and let me know if that helps?

@nicklasb
Copy link
Author

Well, almost, now it tries to load: http://localhost:63342/optimalbpm/optimalbpm/broker/ui/jspm_packages/github/OptimalBPM/mbe-nodes@0.4.10.ts
Which I don't mind, in a way, because it would be really great if jspm created .ts-files for TypeScript-packages.

If I rename mbe-nodes@0.4.10.js to mbe-nodes@0.4.10.ts everything works.

(well there are some dependencies of the package that cannot be found, but that is probably something else)

@guybedford
Copy link
Member

Ok great, try running jspm dl-loader --edge again, I've added an adjustment to that.

@nicklasb
Copy link
Author

Great work, that did it!!

Now that that worked, it seems like the packages internal imports doesn't work properly, it doesn't seem like the deps of the package is checked, but that is possibly a completely different issue if one at all.

Thanks!!

@guybedford
Copy link
Member

👍

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

2 participants