Munter / express-systemjs-translate

Express middleware to speed up systemjs development loads by running translations serverside
28 stars 4 forks source link

issue with systemjs packageConfigPaths configuration. #187

Open aindlq opened 8 years ago

aindlq commented 8 years ago

I'm trying to use jspm with typescript transplier and decided to try this library to speed-up my builds. But it seems that there is some issue with file resolution, I'm getting exception like:

Error loading /Programming/test/jspm_packages/github/frankwallis/plugin-typescript@5.0.9.json.json

Correct file that should be loaded is /Programming/test/jspm_packages/github/frankwallis/plugin-typescript@5.0.9.json so it seems that something adds additional json suffix.

It seems that issue is somehow related to packageConfigPaths property in systemjs, in my jspm config it is

  packageConfigPaths: [
    "github:*/*.json",
    "npm:@*/*.json",
    "npm:*.json"
  ],
aindlq commented 8 years ago

Forgot to add that I'm using jspm 0.17, that could be the reason ...

aindlq commented 8 years ago

From deeper look it seems that this module doesn't work nicely with custom transpliers like plugin-typescript.

aindlq commented 8 years ago

The reason is that express-systemjs-translate should not intercept http request that systemjs sends to get plugin-typescript@5.0.9.json, the assumption at https://github.com/Munter/express-systemjs-translate/blob/master/lib/index.js#L315 is not good enough. As a quick fix I add something like

req.url.endsWith('.json') == false

which is also not good but at least now it works.

gustavnikolaj commented 8 years ago

I don't think this middleware was tested with jspm 0.17 yet. But I think it has been tested with the babel-plugin, and it shouldn't be running the compilation at all in the frontend.

Can you provide a minimal example repo, which reproduces your error? Then it will be a lot easier to dig into :-)

aindlq commented 8 years ago

It actually works perfectly fine if I add && req.url.endsWith('.json') == false to if in https://github.com/Munter/express-systemjs-translate/blob/master/lib/index.js#L315 It correctly compiles typescript on the server.

Here is sample repo https://github.com/aindlq/express-systemjs-translate-typescript. Install all dependencies with jspm install and npm install and use node server.js to run the server.

aindlq commented 8 years ago

That is not a bug, that was my fault. I included full jspm.config.js into html page :( Apparently one need to include only part with mappings.

papandreou commented 8 years ago

@aindlq Happy that you managed to get it working :)

It sounds like a gotcha that should be handled or at least documented if referencing the full jspm.config.js used to work, but fails when express-systemjs-translate is active. Can you drop a few lines about what you had to move out?

aindlq commented 8 years ago

see simple example with typescript+react - https://github.com/aindlq/express-systemjs-translate-typescript

I've loaded into the browser everything that is related to path mappings like map, packages etc. And keep everything that is related to loaders, in server config. https://github.com/Munter/express-systemjs-translate/pull/188 helped with splitting ;)

Munter commented 8 years ago

Awesome that you got it working! And thank you very much for the fedback.

My original intention was that the same config could be used both in browser and on server. But then again I have never run any really complex setups with transpiling. I'd like to figure out if there is something we can do in this library to avoid creating manual work for developers who want to implement this project in their development setup. Simpler usage and setup is better :)

aindlq commented 8 years ago

I would say the experience is quite frustrating so far. Maybe it is only me, but I'm really struggling to understand how path/map/packages configuration makes a difference for server/client side compilation.

E.g I stuck with next issue when the same setup doesn't work with bundle: false. Browser requests http://localhost:3000/jspm_packages/npm/react@15.3.0.json but server is actually trying to read express-systemjs-translate-typescript/jspm_packages/npm/react@15.3.0.json.json, I could imagine that again this is because of path resolution ...

see https://github.com/aindlq/express-systemjs-translate-typescript/tree/no-bundle

Munter commented 8 years ago

I dont know if that has anything to do with 0.17 still being in beta, if you misconfigured, if we have a bug or if we simply need to add new behavior to support 0.17

guybedford commented 8 years ago

@Munter I've posted a builder bug for this in https://github.com/systemjs/builder/issues/667.

aindlq commented 8 years ago

@Munter some updates from my side:

1) It seems that it is possible to improve user experience for default configuration (with bundling). One just need to rewrite system.js configuration files end strip-out all transplier and loader configuration. I think it can be done easily, because you already can intercept config loading (e.g for deepCache population.)

2) With bundle: false it is a little bit tricky. There are several issues, one that @guybedford linked, but I've discovered more ... I'll try to create minimal examples to reproduce them.

Munter commented 8 years ago

@aindlq In jspm 0.16 none of this was needed. And I want this project to work out of the box with the configuration the user has. No fiddling around to make things work. Having to adapt config for this tool is an anti pattern and I very much consider that a bug.

Let's see what behavior we have when https://github.com/systemjs/builder/issues/667 is closed and a new release of 0.17 is out

guybedford commented 8 years ago

Ok, just had another look at this, and this is the namespacing problem of package configurations - basically when you request npm:package@x.y.z.json SystemJS itself doesn't know that you're not just referring to a package called package@x.y.z.json that would have a package configuration at package@x.y.z.json.json. The reason for this is that the versioning convention is not part of SystemJS but only in jspm.

So this is a tricky one unfortunately...