make jsviews ts definitions available on DefinitelyTyped

[jonathantisseau]

Hello,

Currently only jsrender is available on DefinitelyTyped.

Could you add jsviews too ?

Thank you for your amazing work!

[BorisMoore]

Hi Jonathan,

Unfortunately, I tried to add JsViews to DefinitelyTyped, but was unable to do so.

I had a fully working typescript definition file for JsViews, with all tests passing, and fully integrated with the JsRender ts definition, which you can find here: https://www.jsviews.com/#typescript. But when I proposed it in a pull request to DefinitelyTyped, it received a review comment suggesting I change to a new UMD syntax. (See here). Unfortunately that proposed change did not work, and I was not able to understand how to make it work. Possibly the reviewer had not realised the nature of the dependency on the already published JsRender ts definition. (Any changes needed to remain compatible with that).

So then I was in the catch 22 situation where I could not implement the proposed change, was not provided clarification by the reviewer, and was at the same time up against a built-in timeout constraint from the DefinitelyType bot.

See here for the ensuing discussion.

It was very frustrating, given that my initial version was, in my view, totally correct and non-breaking.

If you want to test out for yourself my JsViews d.ts definition, and if satisfied propose a pull request to DefinitelyTyped, maybe you'll have 'better luck' than I did. If I am able to sign off as reviewer, then of course I will do so...

Thanks for bringing up this issue....

[jonathantisseau]

Yes I already have your jsviews .d.ts file included in my project and it works just fine.

I read the issue on DefinitelyTyped and I agree with sandersn on his reply. You would save yourself a lot of work and pain if you just publish the .d.ts in your npm package. And of course, since JsViews got rejected, when you will add anything into JsRender definition, the PR could be rejected for the same reason.

Indeed it won't follow the published guidance, but you won't be the only one in this case : axios, dayjs, moment.js, vue.js, ...

I tried it by adding jsrender.d.ts and jsviews.d.ts manually into the npm package and it worked with this little tweek in jsviews.d.ts: /// <reference types="./jsrender" />

[BorisMoore]

I'm not a regular user of TypeScript, so not necessarily up-to-speed on that aspect. If both JsRender and JsViews include d.ts declaration files in the npm package, then would that require removing the jsrender d.ts from DefinitelyTyped? (If not there could be issues and confusion if the two versions got out of sync). But removing it would be breaking for current users, right?

Publishing d.ts files in the npm is good for those that install JsRender and JsViews using npm. But most people probably load JsRender/JsViews directly from jsviews.com or from CDN. Would some typescript users go that route, and need to get the d.ts files from DefinitelyTyped? Or would it suffice for those users to provide the d.ts files directly from https://www.jsviews.com/#typescript, and maybe? also add the d.ts files to the CDN...

[jonathantisseau]

I'm no expert in TypeScript so this answer is made to the best of my knowledge + some Google.

First I was intrigued by doing ts without npm, I googled it and yes you can. I don't really see the point of it, but for those who wants to, you can leave the current .d.ts links on your site for them to include manually in their project.

Second, the DefinitelyTyped jsrender package should simply be specified as deprecated with a message indicating that the correct .d.ts files are in jsrender/jsviews packages.

Finally for those who use CDN, or direct scripts, in the page (that's what I do), the .d.ts file doesn't matter in this case as the browser don't need them. Those files are only needed in dev env (browsers can't execute ts, only js).

[BorisMoore]

I have been looking at adding the d.ts files to the package. Adding jsrender.d.ts to the jsrender package seems to work fine for me, but I have some issues trying to add both jsrender.d.ts and jsviews.d.ts to the jsviews package. You say above that you were able to do that successfully

I tried it by adding jsrender.d.ts and jsviews.d.ts manually into the npm package and it worked with this little tweek in jsviews.d.ts: /// <reference types="./jsrender" />

Can you show me the exact changes you made to package.json, to achieve that, and also how you tested and discovered the need to change the types /// reference in jsviews.d.ts. It would be helpful to me to see what you did.... Thanks!

[jonathantisseau]

At the top of the jsviews.d.ts file, you reference the jsrender.d.ts file (l. 8).

/// <reference types="jsrender" />

As the source path is just 'jsrender', tsc tries to find project/node_modules/@types/jsrender or project/node_modules/jsrender (you can see path resolution using --traceResolution options on tsc). If you then change it to:

/// <reference types="./jsrender" />

tsc will now try to locate /path/to/jsviews/jsrender.d.ts

For the record, and I know it's off-topic, I understand why you provide jsrender and jsobservable within the jsviews npm package but this isn't the proper way to do it. jsviews npm package should declare dependencies to the jsrender and optionally, if standalone, the jsobservable npm packages. They will then be downloaded automatically by npm. By doing so, the jsrender.d.ts would be available via the original reference to 'jsrender' (as it will be found in project/node_modules/jsrender). And you can still provide a full package for CDN or manual download.

With this, the command

npm install --save jsviews

will give

node_modules
├───jsobservable
│   ├───jsobservable.js
│   ├───jsobservable.min.js
│   └───jsobservable.d.ts
├───jsrender
│   ├───jsrender.js
│   ├───jsrender.min.js
│   └───jsrender.d.ts
└───jsviews
    ├───jsviews.js
    ├───jsviews.min.js
    └───jsviews.d.ts

[BorisMoore]

Thanks Jonathan. Here is my take on the scenarios driving the package design.

Most people, of course, simply use JsViews and JsRender in the browser, by referencing the CDN or the files on jsviews.com/#download. (On Browserify or Webpack though, you might load jsviews.js using require("jsviews")).

In addition, some people use jsrender on the server with nodejs.

So those are the primary scenarios. The jsviews package is not used so often, other than in TS scenarios. However the jsviews package is used, indirectly, because CDNJS automatically deploys files to https://cdnjs.com/libraries/jsviews whenever there is a new tagged release of jsviews package. So the set of files included in the jsviews package is partly driven by the requirement that all relevant files be available on https://cdnjs.com/libraries/jsviews - which means jsviews.*, for single file use, and jquery.views.*, jquery.observable.* and jsrender.* for separate files (explanation here).

For those reasons I did not propose a separate jsobservable npm package, or establish a more complex dependency graph between multiple packages. (Theoretically jsobservable can be used without JsRender or JsViews, but that is not a primary scenario. Most people are only interested either in jsrender.js or else in jsviews.js as a single file).

So given the above package design, if we want to add d.ts files to the packages, the only way I have found that seems to work is, as you say, to make jsviews depend on jsrender. I can then include the corresponding index.d.ts in each package:

{
  "name": "jsviews",
  "version": "v1.0.5",
  ...
  "types": "index.d.ts",
  ...
  "dependencies": { 
    "jquery": "^3.0.0",
    "jsrender": "^1.0.5"
  }
  ...
}

That works with reference types="jsrender" (but also seems to work with reference types="./jsrender", not sure why...)

I think the above will also lead to the index.d.ts files to appear on CDNJS, as in "https://cdnjs.cloudflare.com/ajax/libs/jsviews/1.0.5/index.d.ts", which is probably a good thing...

It looks as if the change is non breaking for the main scenarios, but I would have preferred not to make such a significant change (adding the jsrender dependency to jsviews package) at this point. It is a bit strange to include the jsrender.js files in jsviews and to add the dependency... But I didn't manage to get TS to work by including both jsrender.d.ts and jsviews.d.ts in the jsviews package, and not adding the dependency,

[jonathantisseau]

I tried with a new project and it didn't work like I said before. This is how I make it work : tsconfig.json

{
  "include": ["*.ts"],
  "compilerOptions": {
    "allowJs": false,
    "target": "es5",
    "module": "es2015",
    "moduleResolution": "node" // <-- this si needed for tsc to find definitions
  },
  "exclude": [
    "**/node_modules/*"
  ]
}

package.json

{
  "dependencies": {
    "jquery": "^3.4.1",
    "jsviews": "^1.0.4"
  },
  "devDependencies": {
    "@types/jquery": "^3.3.31",
    "jsrender": "^1.0.4", // <-- this is needed for jsviews/index.d.ts to find jsrender,
    // "@types/jsrender": "^1.0.0" // <-- works well too
  }
}

and I added the two index.d.ts in their respective folder

node_modules
├───jsrender
│   └───index.d.ts
└───jsviews
    └───index.d.ts

Here are all the working possibilities I've found : 1) Add only index.d.ts in jsviews and let the user add @types/jsrender to his project 2) Add index.d.ts in jsrender and index.d.ts in jsviews. The user can then add @types/jsrender or jsrender to its devDep. 3) Add index.d.ts in jsrender, index.d.ts in jsviews and jsrender as (dev)dependency for jsviews. Nothing more for the user to do. 4) Merge jsrender and jsviews definitions into one index.d.ts in jsviews package. Nothing more for the user to do.

I personally prefer the third, but the fourth might make more sense to you as jsviews.js is already a merge.

[BorisMoore]

Yes, I agree, those seem to be the available options. And I would also choose 3 or 4. I had not wanted to create a single merged .d.ts definitions file for jsviews (incorporating jsrender .d.ts) but it is true that that is consistent with the way the jsviews.js merges the other .js files. And it would avoid a slightly strange situation of making the jsviews npm package depend on the the jsrender purely in order for the TS scenario to work. So I might go for #4. I'll be leaving this for a few days though, before returning to it later.

I am also considering adding composer.json support in the next update.

[BorisMoore]

I'm going for #4. I have published the update to jsviews.com, and will shortly publish to npm.

See https://www.jsviews.com/#typescript.

Let me know if this does not work for you...