fine-uploader

fine-uploader copied to clipboard

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.

@SinghSukhdeep can you take a peek at this?

@rnicholus Sure I'll take a look

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

@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

@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.

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

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

@rnicholus this should be good to go.

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.

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

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

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.

So, does this there are no breaking changes then?

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.

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.

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"

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

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.

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.

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.

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

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

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.

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.

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.

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.

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.

@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.

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