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

Better support for loader plugins #5787

Closed
kitsonk opened this issue Nov 25, 2015 · 11 comments
Closed

Better support for loader plugins #5787

kitsonk opened this issue Nov 25, 2015 · 11 comments
Labels
Duplicate An existing issue was already created Suggestion An idea for TypeScript

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Nov 25, 2015

Currently TypeScript does not have a good understanding of loader plugins. Both AMD and SystemJS support the concept of loader plugins (though they vary in format). Currently the only way to integrate a loader plugin is specify the full module name as an ambient declaration and describe the shape of what is returned. For some plugins, this is quite a redundant activity (like a text plugin) since all resources returned by the plugin are text resources.

Both with AMD and SystemJS (also proposed for ES6 module loader), plugins do not have to return a consistent shape. Because of this, I think there are two approaches:

  1. Create resolution logic that supports some sort of glob pattern for ambient declarations, with resolution of the most specific declaration.
  2. Integrate more robust functionality in understanding plugins and what shape they return.

For background reading here is the specification of each:

For SystemJS there are four methods that can be overridden the are exposed by the plugin module:

  • locate - Allows a return of an alternative location for the resource.
  • fetch - Allows the plugin to fetch the "raw" resource.
  • translate - Allows the plugin to modify the resource (e.g. translate on the fly from CoffeeScript to JS)
  • instantiate - Called when the loader actually tries to instantiate the module.

For AMD there:

  • normalize - Allows the resource ID to be modified
  • load - Allows hooking into the actual load and instantiation of the module

RequireJS has support for a couple more, which are specifically geared for building code, but they are not officially part of the AMD standard:

  • write - Write out the module
  • onLayerEnd - Allows a hook into when a layer is finished
  • writeFile - Hook when the file is written
  • pluginBuilder - Substitute another module for this module when doing a build

Also there is a big difference between AMD and SystemJS in the format of the MID for plugins, annoyingly (I won't get on my soap box about how we reinvent the wheel in on the web). AMD is [plugin]![resource] and SystemJS is [resource]![plugin].

@mhegazy asked if plugins change their shape. My personal experience is with AMD and in that case, in practice, some plugins do change their shape (for example in Dojo we have a module that loads CommonJS modules called dojo/node) and others that never change their shape (for example dojo/text loads text resources) and ones where the resource ID determines what is loaded (for example dojo/has take a feature flag and a ternary operator to determine what it loads).

My personal feeling is that option 2 would be horribly complex, likely to have all sorts of edge cases, and while highly useful for those using plug-ins, might just become too challenging. If/when the ES6 Loader is finalised and there is plugin support in there, then having a feature rich analysis of those plugins might be worth the effort. Until then, I think option 1 would allow people to achieve the most use cases with a minimum amount of complexity.

This is related to a discussion that came up in #5759.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

thanks @kitsonk for the detailed writeup. @vladima is working on #5039, not sure how this proposal would fit in there. how do you propose to integrate option 1?

For linking purposes, this is the same issue as #2709, just an alternative approach to solving the issue.

@mhegazy mhegazy added Suggestion An idea for TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Dec 1, 2015
@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 1, 2015

I have what is in my mind, I just don't know how practical to implement in the compiler, the most straight forward would have a simple string glob with matching specific to least specific, for example:

In the .d.ts:

/* for AMD */
declare module "json!*" {
    let json: any;
    export default json;
}

declare module "json!foo.json" {
  let json: { foo: string };
  export default json;
}

/* for ES6/SystemJS */
declare module "*!json" {
  let json: any;
  export default json;
}

declare module "baz.json!json" {
  let json: { baz: string };
  export default json;
}

In module:

/* Under AMD */
import bar from "json!bar.json"; // bar of type any
import foo from "json!foo.json"; // foo of type { foo: string }

/* Under ES6/SystemJS */
import qat from "qat.json!bar.json"; // qat of type any
import baz from "baz.json!bar.json"; // baz of type { baz: string }

The other thought that came to mind, which would be more flexible, but might not be as easy for the average developer to wrap their mind around, is allowing a module name as a regular expression instead of a string. Something like:

declare module /^json!/ {
  let json: { foo: string };
  export default json;
}

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

that is not too bad. this is can be 2 extra lookups for failed module imports with a bang in them.. so in resolveExternalModuleName:

if (!isRelative) {
    const symbol = getSymbol(globals, "\"" + moduleName + "\"", SymbolFlags.ValueModule);
    if (symbol) {
        return symbol;
    }
    // Add handling for module loader extensions
    if (moduleName.indexOf("!") >= 0) {
        let parts = moduleName.split("!", 2);
        const symbol = getSymbol(globals, "\"" + parts[0] + "*" + "\"", SymbolFlags.ValueModule) ||
            getSymbol(globals, "\"" + "*" + parts[1] + "\"", SymbolFlags.ValueModule);
        if (symbol) {
            return symbol;
        }
    }
}

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

@vladima, @weswigham and @RyanCavanaugh thoughts?

@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 1, 2015

One additional thought, if I had my wish (which I don't think your example covers) is that the * is truly a wildcard. The reason I say this is that there are a use cases of complex chaining of plugins. For example in Dojo we have the dojo/has which the MIDs can look like this:

"dojo/has!feature-flag?module/native:module/shim"

Where if the feature-flag is true then the module/native gets loaded and module/shim gets loaded if it is false. They both would have the same API, but I would want to specify my ambient declaration this way:

declare module "dojo/has!feature-flag*" {
    const api = require("module/native");
    export = api;
}

Obviously though, it appears just generic matching isn't straightforward given the current implementation, and the number of features and the combinations of modules is fairly finite. The big upside would clearly being able to load resources like text, CSS, JSON, etc... without having to specify every possible MID.

@weswigham
Copy link
Member

It seems okay - I'd like the regex more, personally, since I think it'd allow more flexibility in the future (given that we don't know how module loaders will evolve/what crazy plugin witchcraft may depend on the string contents of the module name), though I know it'd be a bigger perf hit to utilize - though it would mean we'd understand it syntactically rather than by convention.

@frankwallis
Copy link
Contributor

Would this be in addition to, or instead of, the import myTemplate: string from 'my-template.html' syntax proposed in #2709?

Perhaps there are technical reasons why the above is easier to implement, but the typed import proposal seems much simpler from a user perspective and it appears to have quite a lot of demand.

@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 7, 2015

I think the challenge with #2709 is that it implies some sort of runtime loader. I am afraid resources don't just get loaded by magic. This proposal allows you to imply, via an ambient module, how a resource like my-template.html would actually be handled by the loader, which is external to TypeScript and it would work for the types of loader that TypeScript is compatible with.

For example, what would be the type of the following:

import anObject from 'anObject.json';

You actually need to understand what the loader will do... Is that a string? Is that a structured object of which you can actually type? Maybe with an HTML resource, the loader is going to create an unattached DOM node and going to set the test on that and return to you the parsed nodes. Yes, you can make a type annotation of it, but how do you know that would match the loader is doing? This is why module loaders use plugins. Using SystemJS it would be something like this:

import anObject from 'anObject.json!json';

At that point, we already have a mechanism for describing external modules like that. So I think the danger in my opinion with #2709 is that it sort of implies that there will be some sort of magic intelligence that is going to properly load the module. They could be used in combination, but then they would be redundant and #2709 wouldn't solve the problem of describing what the plugin is actually doing. Anyways, that is my opinion.

@frankwallis
Copy link
Contributor

I think I understand what you are saying - if you have a setup where you are loading json files as strings and you move that code to a setup where you are loading them as objects then when using these definitions you will get a compiler error if you update the loader ambient declarations. If you don't update the loader declarations then you will get a runtime error in just the same way as you would with the typed imports.

Ultimately your code is going to rely on the fact that the import is a string and not an object so you are going to need to change something. In the case of a json file then the type of the imported object will be specific to your code (e.g. interface IMyConfig { debug: boolean }, so these ambient declarations will belong with your code, and not with the loader environment.

I don't think that the complexity of this new configuration system is worth the benefit, and that given both options > 90% of people would just use import myTemplate: string from 'my-template' and import myConfig: IMyConfig from 'myconfig.json' because it's easier.

@mhegazy mhegazy added In Discussion Not yet reached consensus and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Dec 7, 2015
@nillis
Copy link

nillis commented Jan 21, 2016

Is there already some proposal being accepted/developed?

FYI I currently use this in an Angular/SystemJS/JSPM application to load templates which enables me to create one bundle containing everything. At the moment I just use template url's which I inline with https://www.npmjs.com/package/gulp-angular-embed-templates as a workaround.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 25, 2016

the wild card looks like the way to go. closing this in favor of #6615.

@mhegazy mhegazy closed this as completed Jan 25, 2016
@mhegazy mhegazy added Duplicate An existing issue was already created and removed In Discussion Not yet reached consensus labels Jan 25, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants