Updated Typescript to support canonical imports

[fullykubed]

Brief description of the changes

Solves issue explained in #1838. Adds support for the canonical Typescript imports. Matches the existing import syntax shown in the import description page except that the qq can be dropped as it now properly knows that qq does not exist on the global namespace (due to the UMD modular design).

Typescript example:

// use Fine Uploader core (non-ui) for traditional endpoints
import qq from 'fine-uploader/lib/core'

const uploader = new qq.FineUploaderBasic({...})

becomes

// use Fine Uploader core (non-ui) for traditional endpoints
import {FineUploaderBasic} from 'fine-uploader/lib/core'

let uploader = new FineUploaderBasic({...})

Couple of notes:

• To avoid confusion in the future, I would add some typescript instructions into the import description page.
• Since Typescript gets rid of qq on the global namespace, all of the utility functions that were on there are not visible to Typescript users. I assumed this was fine as it appears they are only being used internally.
• It doesn't look like fine-uploader.test.ts hooks up to any runtime tests which I think was the root cause for this issue originally. I did not want to mess up your test flow (and npm test kept failing on the clean repo for me), but they should be fairly trivial to make.

What browsers and operating systems have you tested these changes on?

Chome 58
Typescript 2.3

Have you written unit tests? If not, explain why.

Updated the fine-uploader.test.ts file to show modularized Typescript syntax

[singhjusraj]

I see that the def file has changed considerably. It will take me some time to get through this.

[fullykubed]

As a note to aid in your review, I fortunately didn't change any of the definitions of any of the functions. I just exposed the js classes as Typescript classes, and encapsulated into them the appropriate functions (based on the online API). This was done since Typescript users won't have access to the qq global variable after they transpile their ts to js -- Typescript users will usually target a module system like AMD, CommonJS, or ES6 and the UMD design of the project will then not expose everything under a qq variable but rather on an export object. That's why the Typescript docs recommend using the module template ( https://www.typescriptlang.org/docs/handbook/declaration-files/library-structures.html ). I am not sure why having it the current way would have allowed rollup to work with Typescript as it seems it should have still broken it. If you want to use these new Typescript definitions with Rollup all you'd need to do is change the module option of the tsconfig.json file to "ES6." While I haven't tested it, this will give you the import/export syntax that should allow rollup to work. What I do know, however, is that the current definitions will trick the Typescript compiler into allowing code that will lead to runtime errors. Let me know if I can help in any way. Jack

…

On Thu, May 25, 2017 at 9:01 PM Sukhdeep Singh ***@***.***> wrote: I see that the def file has changed considerably. It will take me some time to get through this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMr1ar6fywffvpVMnKvAy5Dj5Y1eu-BNks5r9iRVgaJpZM4Ng9kV> .

[singhjusraj]

Current structure is set in a way that you have to obviously include fine-uploader JS file globally in your index.html and provide support to be able to use the same syntax for TS projects.
And hence we wouldn't need Rollup or any other module bundler/loader to load the library, that's how it is working right now.

I totally understand what you are saying or trying to implement here. But I'm afraid it's not going to work with Rollup as of yet.
I haven't tried your changes with Rollup yet, I will soon.

Also just a note that Rollup isn't my obsession, it's what is recommended by Angular team itself and in all their documentation. Meaning that there are lots of people out there using exactly that myself included.

[fullykubed]

Ah, I see what you are saying: there should be a both an import statement in the Typescript and a global include in the index.html file. That would certainly work, but it would be nice to just have to import the module once in our Typescript projects which is the functionality that the new .d.ts file provides and is recommended by the Typescript crew. Also, this would work much better with bundlers (including Rollup, I think).

Also, Rollup is certainly a wonderful utility (just haven't used it personally), and I did not mean to imply in any way that you should not love it and support it. What I am trying to say is that when transpiling Typescript, it will produce javascript that does not expose the global qq variable. Thus, you will get a runtime error regardless of whether you use Rollup or not -- unless, as you pointed out, you do a second include of the underlying javascript at a different point in your html code.

If you think I am wrong (and I certainly could be!), could you provide an example of how you are using Typescript with Rollup and not getting resulting runtime errors? I might be missing something.

I think that this whole issue is probably a result of the import documentation page. It says that ES6 import syntax can be used with languages like Typescript and utilities like Webpack and gives several examples -- none of which would work in Typescript. That's what led me to develop the new Typescript definition file that now follows the Typescript's team's recommendations for UMD projects and will work out of the box with the instructions in the documentation.

[rnicholus]

The document you are referring to was written by me, before TS support was added. And I don't personally use TS, so it's likely we need dedicated TS docs too.

[fullykubed]

No worries! Definitely hard to keep up with everything popping up every other month. Fortunately, the original instructions should be 100% fine -- we just need to have the .d.ts file treating the modules as modules rather than using the global namespace (which won't have anything on it since fine-uploader uses UMD).

[singhjusraj]

I haven't forgotten about this. Haven't had any time to work on this.
I'm hoping to dedicate some time this weekend.

[singhjusraj]

Did some testing and I've put together a repo here where all the changes are seems to be working with SystemJS and Rollup (Thanks to improvements rollup's commonJS plugin).

I need to do some more testing on a real Angular 4 & TypeScript project and then we can go further from there.

I am optimistic that this will be a great benefit to Angular & TypeScript based projects, but is also a major syntax change for the existing users who are using globally scoped FineUploader JS with current TS definitions.

Also we need to document this change properly and maybe have a dedicated page or section for Angular 2+ and TypeScript integration.

[fullykubed]

Awesome! Let me know if I can help with anything. My two cents would be just having an example import statement for TS on the documentation for importing would be enough. Everything else should function the same way. Best, Jack

…

On Sun, Jun 11, 2017 at 4:42 PM, Sukhdeep Singh ***@***.***> wrote: Did some testing and I've put together a repo here <https://github.com/SinghSukhdeep/fine-uploader-ts> where all the changes are seems to be working with SystemJS and Rollup (Thanks to improvements rollup's commonJS plugin). I need to do some more testing on a real Angular 4 & TypeScript project and then we can go further from there. I am optimistic that this will be a great benefit to Angular & TypeScript based projects, but is also a major syntax change for the existing users who are using globally scoped FineUploader JS with current TS definitions. Also we need to document this change properly and maybe have a dedicated page or section for Angular 2+ and TypeScript integration. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMr1aj7QQzN6WXFp_ypM_xEFGYxcjDm0ks5sDFEegaJpZM4Ng9kV> .

[singhjusraj]

I think it'll be better now if we divide the modules into individual files for each and have one index.ts exporting everything since we're now using proper import export syntax.
It'll be easier to maintain and also code reviewing future changes as I'm really having hard time finding out issues such as above

[fullykubed]

Yes. We can split the ambient module declarations into separate files (even splitting separate module elements into their own modules if we want to break it up even further); however, these new modules must have different names than those names in the final index.d.ts file (which need to remain as they are).

At what level do you want to split things into separate files? Current module level? Class/interface level?

[singhjusraj]

I think splitting by current module level should be okay for now, we can always split it further if needed.
I'll take a further look at all the declarations to make sure they are all as they were.

Can you also make sure all the interface declarations are as they were and they're extending other interfaces properly.

Thanks for all your contribution so far

[singhjusraj]

In addition to

'fine-uploader/lib/core'
'fine-uploader'
'fine-uploader/lib/s3'
'fine-uploader/lib/azure'

also need to declare the 'fine-uploader/lib/all' module which would essentially have all the flavours

[irbian]

Hi!

I have read some of your threads regarding the integration with angular 2. Is this one the most up to date? We are very interested on integrate this library with our project because we need the IE9 support

[singhjusraj]

@irbian as you can see in this PR we're in the middle of transforming our TypeScript support.
If you want to use FineUploader in your project at the moment, look at this file for syntax.
However it will change for better after we merge this PR.

[singhjusraj]

Can you please address those things

[singhjusraj]

@jclangst Are you still pursuing this PR?

If not, the things that I mentioned in my last review, I'll have to revert those back to what it was before and then merge this. I'm hoping to have this released very soon as many users are requesting this

[singhjusraj]

@rnicholus Can you please review the documentation changes from this PR and also qq and its related functions are no longer available for TS users, so slight syntax change is required for all TS users.

I have updated the documentation to reflect how library should be imported in TS, is there anything else we need to do before we merge this?

[rnicholus]

Are these the breaking changes you mentioned to me earlier? If so, looks like this only affects Typescript users. Is this correct?

Docs look fine to me. I generally only use let if I know a variable needs to be changed, which means I default to const instead. But that's really a nitpick here.

Feel free to merge and release. Please do familiarize yourself and follow the release (and also the merging) guidelines at https://github.com/FineUploader/fine-uploader/wiki/Maintainers-guide first.

[singhjusraj]

Yes this only affects TypeScript users, no one else.
let is mostly used in TS instead of using var directly, but it actually compiles down to var, so don't worry about it.

I've read the maintainers guide before,
If I understand correctly, following are the steps:

1. Update the version number in package.json and version.js
2. Then do squash and merge this PR into develop branch
3. Then merge develop into master
4. Run publish Makefile recipe from master branch, which would publish to npm.
5. Create a new Release in the Releases section following the instructions

Step 6 in the Releasing steps about customize.js is bit unclear to me.

And what should be the next version number 5.14.6 or 5.15.0 since this is major change for TS users. I see you already have few other PR's on 5.15.0 so I'm guessing it'll probably be 5.14.6.

[rnicholus]

I'm fine with let, but my point is const is usually more appropriate. Both compile down to var, but const gives you additional safety checking at build time.

Let's continue to follow semver when deciding version number. If this fixes a bug only, patch version increment. If it's more if a feature, minor version increment. Can you comment on the scope of breaking changes for typescript users? How substantial are the breaking changes?

[singhjusraj]

It's as described in the PR description. Changes to the import statement as outlined in the doc update and all of the utility functions being used like qq.functionName won't be available.

You told me once that you are planning to remove those in future releases, so I guess that's fine. Besides TS users really don't need those functions.

import qq from 'fine-uploader/lib/all' also isn't available because the global namespace qq isn't there.

So essentially in order to use all flavour together, separate import is required for each flavor.

Other than those changes, there isn't any change in the syntax other than what's described in the PR description.

So in order to release this as v5.15.0, need to move the PR's currently under this tag to another milestone

[danopkeefe]

@SinghSukhdeep I noticed 2 things regarding the TS canonical imports:

1. 'allowEmpty' seems to be absent from interface ValidationOptions
2. I have not been able to successfully instantiate a new PromiseOptions as done here: https://github.com/FineUploader/fine-uploader/blob/5.15.3/client/typescript/fine-uploader.test.ts#L94

Possibly, both of these are misunderstandings on my part.