Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

fix: TypeScript declaration for lib/core/s3; add retry option #1989

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bradleyayers
Copy link
Contributor

@bradleyayers bradleyayers commented Mar 27, 2018

Brief description of the changes

  • Adds TypeScript declaration for lib/core/s3.
  • Adds retry option to FineUploaderBasic.

Resolves #1988.

Extra package.json files have been added to commonJs to associate types with nested modules, this follows the pattern used by date-fns.

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

n/a

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

fine-uploader.test.ts has been extended.

@rnicholus
Copy link
Member

@SinghSukhdeep can you take a peek at this?

@singhjusraj
Copy link
Member

@rnicholus Sure I'll take a look

Copy link
Member

@singhjusraj singhjusraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take another deeper look soon. Following is what I've noticed so far.

/**
* RetryOptions
*/
retry?: RetryOptions;

This comment was marked as spam.

/**
* Retry options
*/
retry?: UIRetryOptions & RetryOptions;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -1,3 +0,0 @@
"use strict";

module.exports = require("../fine-uploader/fine-uploader");

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@singhjusraj
Copy link
Member

singhjusraj commented Mar 30, 2018

We should also update the azure module similarly as that is also missing the core module declaration.
Something I missed out previously

@singhjusraj
Copy link
Member

@bradleyayers Just wanted to check your thoughts on possibility of 'fine-uploader/lib/all' in TS. This is missing and I can't think of a way to have this since we don't have the qq namespace here

@bradleyayers
Copy link
Contributor Author

@SinghSukhdeep I've pushed up a couple of commits attempting to add lib/all and lib/core/all. To be honest I'm not exactly sure what exists in those distributions, so if you can double check I got it right that would be great. It's times like these I hope having an ES module distribution would remove the need for these separate distributions. I'd really like to have the whole code base migrated to TS to make these things easier.

@bradleyayers
Copy link
Contributor Author

@SinghSukhdeep let me know if you're aware of any other issues, else I think this is good to go.

@@ -2691,6 +2693,14 @@ declare module "fine-uploader" {

}

declare module "fine-uploader/lib/all" {
import { azure } from 'fine-uploader/lib/azure';
export * from 'fine-uploader/lib/core';

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@singhjusraj
Copy link
Member

Hey sorry I was away for couple days. I think it all looks great except for one line I mentioned above

@bradleyayers
Copy link
Contributor Author

@rnicholus this should be good to go.

@rnicholus
Copy link
Member

Everything looks fine here, but I'm not sure I understand the documentation changes. You shouldn't have to explicitly reference an index.js file when importing a module, unless I'm missing something. If that is the case here, then this would be a very significant breaking change, and we'll have to figure out how to update the code to prevent that.

@bradleyayers
Copy link
Contributor Author

Fair call, this might be better suited for release/6.0.0?

@rnicholus
Copy link
Member

Is it really necessary to reference index.js directly? I’ve never had to do that myself.

@bradleyayers
Copy link
Contributor Author

The file extension references are only for the bundler configurations, which I don't have direct experience with (e.g. rollup). Nothing will change with respect to standard JavaScript imports.

@rnicholus
Copy link
Member

So, does this there are no breaking changes then?

@bradleyayers
Copy link
Contributor Author

So, does this there are no breaking changes then?

"Breaking change" is a subjective term, so I think this boils down to being a judgment call. I'm happy to call this not a breaking change, even though if people are relying on exact file locations for custom build configurations they'll need to make some changes when upgrading. If you'd rather err on the side of calling that a breaking change, then I can target this to release/6.0.0.

Without an unambiguous definition for breaking change this decision just needs to be a judgement call made by a maintainer.

@rnicholus
Copy link
Member

rnicholus commented Apr 2, 2018

If any user using the library in an expected/documented manner needs to adjust their code to upgrade, then the version contains breaking changes, and that must always be avoided in minor or patch releases, per semver spec. I’ll take a closer look at these changes soon to better determine if the documentation changes are necessary and if any breaking changes are present or necessary.

@singhjusraj
Copy link
Member

Just to weigh in, we only need to specify file extensions for module loaders like SystemJS (not sure about Webpack) and module bundlers like Rollup (again not sure about Webpack).
As far as normal import export is concerned, having or specifying index.js in the docs doesn't matter.

So only a slight modification is required in the bundler/loader configs.

And in our package.json
"main": "lib/traditional.js" would also needs to be changed to "main": "lib/traditional/index.js"

@bradleyayers
Copy link
Contributor Author

@SinghSukhdeep does pkg.main support having the file extension omitted?

@rnicholus
Copy link
Member

I think i'm ok with this provided the documentation changes are reverted. But I do want to do a bit of manual testing myself first. Thanks both of you for all of your hard work.

@bradleyayers
Copy link
Contributor Author

Reverting the documentation changes would also require reverting the code changes that move lib/traditional.js to lib/traditional/index.js, else the docs won't match up with the file structure.

@rnicholus
Copy link
Member

I’m personally fine with just leaving the docs as-is (before any changes). It’s common to use the index.js pattern (I’m using it in several closed and open source projects) and I’ve never seen docs explicitly reference the index file.

@rnicholus
Copy link
Member

I’m fairly certain everything will just work without making any code changes in webpack environments. I’ll have to confirm though. But if indeed this does break for users of rollup, then without reverting the breaking code changes, this will have to sit and wait to go out with 6.0

@rnicholus
Copy link
Member

To ensure we don't break backwards compatibility, can't we just keep the traditional.js, s3.js, etc files?

@bradleyayers
Copy link
Contributor Author

To ensure we don't break backwards compatibility, can't we just keep the traditional.js, s3.js, etc files?

Yeah I considered that, but I wasn't sure if the module resolution spec includes whether to try directory or file first when resolving. It seemed like a risky approach that could be broken in the future. It's probably worth checking though.

@rnicholus
Copy link
Member

Well, if both /traditional/traditional.js and traditional/index.js point to the same location, there shouldn't be an issue, unless I'm missing something. We could eliminate traditional.js in 6.0, but this allows us to ensure there are no breaks in 5.x.

@bradleyayers
Copy link
Contributor Author

I'm assuming that for package.json to have effect the directory index needs to be used by TypeScript. I'll have a play with this and report back.

@bradleyayers
Copy link
Contributor Author

I've tested this and leaving the .js files in-place, and adding package.json in subdirectories seems to work. I'll update the PR to use this approach to minimise the changes.

@singhjusraj
Copy link
Member

Hi @bradleyayers are you still pursuing this PR. We did a lot of work on this and it would be great if we could merge this.

@bradleyayers
Copy link
Contributor Author

@SinghSukhdeep I'm not actively working on it, but I am still using my FineUploader fork in production (it includes this change + using native promises), so I'm still invested in seeing this succeed.

@singhjusraj
Copy link
Member

Awesome!
I will look into this again and see what is missing and hopefully we can get this through

@singhjusraj singhjusraj self-assigned this Oct 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants