filestack / filestack-rails

Official Ruby on Rails plugin for Filestack File Picker that makes it easy to add powerful file uploading and transformation capabilities to any web or mobile application.
https://www.filestack.com
Apache License 2.0
223 stars 101 forks source link

"v3" loads wrong JS #216

Closed bkroeker closed 5 years ago

bkroeker commented 5 years ago

<%= filestack_js_include_tag %>

loads //static.filestackapi.com/filestack-js/1.x.x/filestack.min.js

but it should, I expect, load //static.filestackapi.com/filestack-js/3.x.x/filestack.min.js

My code wasn't working until I manually loaded the latter URL and bypassed the filestack_js_include_tag method.

gabifija commented 5 years ago

Hi @bkroeker ! I have updated filestack-rails to version 5.0.0. By default we setup filestack-js version to 3.x.x. You can also provide a version which suits you best. If you want to change the version, you have to add specific version in configuration. Here is an example: https://github.com/filestack/filestack-rails#filestack-picker-version

bkroeker commented 5 years ago

@gabifiolek Thanks! Seems to work well.

bkroeker commented 5 years ago

@gabifiolek One thing I'm noticing is that a bunch of my rails tests began failing. I tracked it down to Picker#url_exists? which was swallowing the VCR error that was being thrown. The fix, of course, is that I need to enable vcr on some additional specs, but it would have been nice to have a more accurate error message than "Incorrect config.filestack_rails.version". Maybe consider scoping the rescue to only catch URI::InvalidURIError or something?

That said, I'm not particularly thrilled about my site making a request to check if "3.x.x" is valid on every single page load. I'd much prefer if the filestack-rails gem just ran it against a regex or something.

gabifija commented 5 years ago

@bkroeker Thank you for your comment!

I was thinking of a regex to check if the version is correct. But, the problem is that a regex has to be updated each time when we release filestack-js version. For instance: when we update filestack-js to newer version - 5.2.1, regex will fail. I can not create a regex for random digits, because file url (https://static.filestackapi.com/filestack-js/#{version}/filestack.min.js) doesn't not exist for random version. URL works only for specific versions. That's why I have to check if the URL exists, and if we have an access to it.

Does it make sense?

What I can do in that case - I can move checking if url_exists? to configuration. It will not check it every single page load. https://github.com/filestack/filestack-rails/pull/219/files ?

Let me know if I can be any further assistance.

gabifija commented 5 years ago

@bkroeker FYI, I moved url validation to configuration in filestack-rails 5.1.0. Let me know if that works for you better. Any suggestions always welcome.

bkroeker commented 5 years ago

@gabifiolek Yeah, I totally get the reason why you went that route rather than a regex, but IMO it's coding too defensively and the hidden side effects aren't worth it. You're holding developers' hands more than is warranted.

Scenario 1: Let's say you validated via a regex. What would happen if the user typed in 3.9.9 and that version didn't exist? Their page would list a 404 that //static.filestackapi.com/filestack-js/3.9.9/filestack.min.js could not be found. They'd say "Oh, why doesn't that file exist?", they'd look it up, and find a version that does exist.

In this case, the regex would be just a sanity check to make sure they're not typing in something totally bogus. A bit more responsibility would fall on the user to make sure their version is valid, and I think that's fine. You could put a link on the readme to a place where they can look up the available versions.

Scenario 2: let's say you kept the current code, and there's a new major version of filestack released: 4.x.x, and it has some backwards-incompatible changes to its api. Your current code that checks for its existence would pass, but the gem could break despite its attempts to be foolproof.

With a regex like /[123]\.(x|\d+)\.(x|\d+)/, you could control for major versions. 4.x.x would not be considered valid until you've tested it, fixed any broken behaviour, and updated the regex. Once the regex is updated, everyone would know that it's been tested and is ready for 4.x.x. This is a safer and better solution, in my opinion.

Current Scenario: Now, to the changes you just made for 5.1.0, I don't think the behaviour actually changed despite your changes to code. Ultimately, the url validation code is still triggered by the call to filestack_js_include_tag, which means it's triggering on every single page load. I will continue to avoid calling filestack_js_include_tag and load the js manually to avoid this behaviour.

Thanks for listening.

gabifija commented 5 years ago

@bkroeker Thank you for giving me a feedback! I really appreciate it.

I've talked with our team and we decided to remove version validation (in 5.2.0) I left a comment in README to check available filestack-js versions.