frankwallis / plugin-typescript

TypeScript loader for SystemJS
MIT License
248 stars 47 forks source link

Configurable support for file extension fall back #176

Closed mavericken closed 7 years ago

mavericken commented 7 years ago

This change adds configurable support for trying multiple file extensions when importing.

  typescriptOptions: {
    "fileExtensions": ["ts","tsx","jsx","js","json"]
  },

If the original extension that SystemJS asked for exists in the list, we try that first. Otherwise, we try each extension in order until we find the file. If we fail to find the file, we go ahead and return the original address to the missing file, like before.

frankwallis commented 7 years ago

Thank you for creating this, I agree it is a big problem with the plugin as it stands.

My concern here is that when loading in the browser this will be triggering lots of 404 errors and may cause performance issues. Are you using the browser loading functionality or just bundling the application in nodejs?

mavericken commented 7 years ago

I am developing with browser loading at the moment, but my intent is to bundle it for production releases. The way I see it, the cost goes away for production bundles, and for development it is a choice of whether you want gotchas to dictate your directory structure or you want to pay for extra file lookups when you need them. Even for development using this option, you could optimize directories by setting their defaultExtension appropriately, it will first check the requested extension if it is in the list. If the defaultExtension matches, no extra 404 errors.

detrohutt commented 7 years ago

@mavericken I'm trying to use your fork and it doesn't seem to do anything for me.. I've added the fileExtensions option under typescriptOptions but it still just tries the default extension(from defaultExtension option) and then returns a single 404.

Looking at the code I see you've declared a couple new functions but I don't see how/where they're called.. am I missing something?

mavericken commented 7 years ago

@detrohutt I implemented a "locate" export. Locate is used by SystemJS to figure out where to fetch the file, so SystemJS itself should be calling locate. See https://github.com/systemjs/systemjs/blob/master/docs/creating-plugins.md

Even when this all works correctly, you will still see 404 errors for the files it tried before it reached the correct working one. If you make your list ['badextension','anotherbadextension','somanybadextensions'], you should be seeing a 404 for each of them before failing.

Edit: I actually just experienced something that seems similar to your description. I did some adjustments to my JSPM packages, and somehow the plugin-typescript@5.2.7.json became an empty file. With the "main":"plugin.js" not in there, I get a single 404 error attempting to fetch jspm_packages/github/frankwallis/plugin-typescript@5.2.7. Perhaps there is a JSPM bug that caused this in both of our cases.

detrohutt commented 7 years ago

@mavericken Thanks for the reply. This is my first time using JSPM so I may be doing something stupid.. I installed your fork as a github dependency. Is that the correct way to go about it? I checked my plugin-typescript@5.2.7.json file and it contains this:

{ "main": "plugin", "format": "register" }

I also added several bogus extensions as you suggested and it's definitely not trying any of them.

Edit: I just realized why it's not working for me. I looked at the plugin.js file in the jspm_packages/github/mavericken/plugin-typescript@5.2.7 folder and it doesn't have your changes.. could it be because it's using the 5.2.7 tag maybe?

Final Edit: Ok, it was just the tag. I changed it to github:mavericken/plugin-typescript@master and it works great now! Thanks for making it. :)

detrohutt commented 7 years ago

I'm not sure if this would be considered a bug, but this breaks hot-reloading(using systemjs-hot-reloader) in this scenario: The defaultExtension option is set and the file's extension is not the default extension

In other words, any time tryExtensions is utilized, the hot-reloading "connection" is broken(or never made) for that file. Other files using the default extension still hot-reload normally. I assume this has something to do with the systemJS registry but I don't know much about that.

Edit: After some debugging and research, I found that the problem is that the moduleName in the System._loader.moduleRecords always uses the defaultExtension, so when you later try to reload it by the "new" moduleName(with the correct extension) it can't be found in moduleRecords.

It looks like you would possibly need to use the instantiate hook to create the module manually with the correct name. Otherwise it may make more sense to use the normalize hook but that isn't available to plugins.

mavericken commented 7 years ago

I suppose I should have mentioned related efforts in this pull request, so I could have saved you some time on that research. https://github.com/alexisvincent/systemjs-hmr/pull/4

That pull request should fix cases where locate finds a different file, like here. It is fresh off the presses though, so for this to get all buttoned up, we will need systemjs-hmr to label, and systemjs-hot-reloader to update the dependency and label.

detrohutt commented 7 years ago

Ah, I see. Thank you! systemjs-hot-reloader actually just recently switched to using systemjs-hmr on their master branch, which I'm currently using.

Edit: Aha! I just needed to run jspm update capaj/systemjs-hot-reloader and now everything is working great!(as long as running @master versions of both your plugin-typescript fork and capaj/systemjs-hot-reloader) :) Thanks for pointing me in the right direction @mavericken 👍

mavericken commented 7 years ago

So I ran into a problem with this today. This solution assumes that address will be equal to the result of the locate call, and for reasons I can't yet explain, that stopped holding true in my project.

Edit: I figured it out. Bundle arithmetic subtraction will not remove a module if it's address does not match its name, so my attempts to hot reload failed because it was included unbeknownst to me in my bundle. Also noticed that it is still necessary to include both ".tsx"{"loader":"ts"} and ".ts"{"loader":"ts"} configs or it won't get transpiled.

unional commented 7 years ago

This sounds great!

Without this, the current approach (that I'm aware of) is to organize files based on their extension:

  packages: {
    "app": {
      "main": "index",
      "defaultExtension": "ts",
      "meta": {
        "*.ts": {
          "loader": "plugin-typescript"
        }
      }
    },
    "app/components": {
      "defaultExtension": "tsx",
      "meta": {
        "*.tsx": {
          "loader": "plugin-typescript"
        }
      }
    }
  }
mavericken commented 7 years ago

I noticed somewhat of a bug with this feature. Normally when you bundle stuff, you can still hot-reload individual pieces that were included in the bundle. With this feature, the bundle simplifies away the extension shenanigans, so the hot reloader is presented with the original address instead of the fallback address. Without the correct address to check, it becomes unclear that a change to the fall back file should be reloaded.

mavericken commented 7 years ago

I came up with a better way to handle multiple file extensions: https://www.npmjs.com/package/one-plugin