Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fileIgnorePattern option #109

Merged
merged 2 commits into from
Aug 20, 2018
Merged

Add fileIgnorePattern option #109

merged 2 commits into from
Aug 20, 2018

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Jul 19, 2018

What Changed & Why

This PR adds the option to specify fileIgnorePattern, to ignore certain files (like sw.js).

Usage:

s3: {
  // other configuration
  fileIgnorePattern: 'sw.js'
}

Related issues

This is an alternative solution for #97.

PR Checklist

  • Add tests
  • Add documentation
  • Prefix documentation-only commits with [DOC]

@achambers
Copy link
Member

Thanks for the contribution @mydea . I have the same reservation with this PR as I did with #97 and that is this:

This kind of breaks how deploy plugins config works. It is a feature of ember-cli-deploy that you can specify a config property as a function which will be evaluated at runtime with the current deploy context object passed in as an argument.

This PR introduces the concept of a "config property as a function" that performs in a fundamentally different way than the functions do for all other plugin configs and it's at odds with the config function convention that ember-cli-deploy employs.

Currently it's well documented that if you supply a function as a config property it behaves a certain way. To magically make an exception to this seems like it could be problematic.

What are your thoughts on this?

Also interested to hear from @lukemelia and @ghedamat too.

@mydea
Copy link
Contributor Author

mydea commented Jul 19, 2018

Well, I personally would find this the most simple, configurable solution. But of course, other approaches would also be fine. We could provide a map, but that might be much harder to do e.g. when using fingerprinted assets. Then again, for me the most pressing need for that is for the sw.js file, which I guess is a common use case. We could also provide a "no cache blacklist", e.g. a glob for files to set the caching to no-cache, like noCacheFilePattern: 'sw.js'. While that would be much less flexible, it would not require a function.

From my perspective, the primary goal should be to make this addon work nicely with service workers (=sw.js). I think allowing a function would provide the most flexibility, but am absolutely open for other solutions - I'd be glad to adjust the PR accordingly.

I also feel that the approach with fileIgnorePattern: 'sw.js', and providing two s3 deploy configs would work, but it feels kind of overkill to me. Then again, if that's what I need to do to get this to work, I'll gladly do it!

@achambers
Copy link
Member

The key thing here is that ember-cli-deploy is built on the convention that you can either provide a value as a config property, or a function. If providing a function, one knows that they can expect that the function will be called with the current context object, followed by a pluginHelper object. To make an exception to this for one use case seems problematic to me.

The concept of aliasing plugins is the ember-cli-deploy convention for running the same plugin with different configs and the current recommended approach for this use case. Sure, it's a slight bit more code, but is that a problem? It's just come configuration.

I'm happy to continue to explore new avenues absolutely and let's continue this conversation if it's of interest to you, but I'm very hesitant to add one off exceptions to the core conventions ember-cli-deploy employs such as is proposed by this PR.

@mydea
Copy link
Contributor Author

mydea commented Jul 19, 2018

So, I'll adjust this PR to instead add a fileIgnorePattern configuration option?

@achambers
Copy link
Member

So, I'll adjust this PR to instead add a fileIgnorePattern configuration option?

I'd be more than happy to merge that change, yep 😄

@mydea mydea changed the title Allow file cache Add fileIgnorePattern option Jul 19, 2018
@mydea
Copy link
Contributor Author

mydea commented Jul 19, 2018

I have updated the PR accordingly!

@ghedamat
Copy link
Contributor

thanks @mydea

I've not been in touch closely but I think it LGTM after the last update

wdyt @achambers ?

@mydea
Copy link
Contributor Author

mydea commented Aug 20, 2018

Any chance to get this merged? :)

@ghedamat
Copy link
Contributor

Sorry @mydea ! we'll be on it shortly!

@ghedamat ghedamat merged commit 336fd17 into ember-cli-deploy:master Aug 20, 2018
@ghedamat
Copy link
Contributor

@mydea released [email protected]

let us know if it works for your needs!

@mydea
Copy link
Contributor Author

mydea commented Aug 21, 2018

Awesome, works for me, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants