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

Rename add()? #1

Closed
mahemoff opened this issue Mar 2, 2012 · 9 comments
Closed

Rename add()? #1

mahemoff opened this issue Mar 2, 2012 · 9 comments

Comments

@mahemoff
Copy link

mahemoff commented Mar 2, 2012

I'd suggest load() or require() is more appropriate, since that's what the programmer is trying to achieve. That it's add()ed to the cache is the magic this library is making invisible.

/nitpick

@nimbupani
Copy link

Nothing should be 'magic'. Given the 2.5MB limit, a programmer should know that things are being added on top of existing resources for the same domain. So, if you have a jquery-1.5.min.js and then a jquery-1.8.min.js, it should be known that it is added and not replaced, or the resource-management is automagically managed.

@mahemoff
Copy link
Author

mahemoff commented Mar 2, 2012

But if it's doing its job, it's not being add()ed with every call; whereas
require() or load() reflect what is actually happening each time you call
it and why you're actually calling it.

@nimbupani
Copy link

I see, perhaps there should be another function, that merely adds? and then the load execs the add if it doesn't exist? A function should ideally be isolated to a singular task.

@mahemoff
Copy link
Author

mahemoff commented Mar 2, 2012

Maybe a separate add(), especially as Mike Taylor and I proposed a remove() elsewhere. Perhaps implemented as an "eval"
option (defaulting true) on this add/load() method rather than a separate dedicated method though, since it's not clear how common a use case it is. And there's also an argument not to add it at all unless there's a real-world use case for it.

@yuchi
Copy link

yuchi commented Mar 2, 2012

In this context I'd like to add my 2 cents: if this pattern becomes popular a plug in for require.js and others could do the job about API names.

@nimbupani
Copy link

In my view, this use of localstorage is not conducive, and efforts should be made to make the appCache better as it is dedicated for caching. I assert localStorage is best used with data (e.g. to-do lists, map points, svg paths(?)) not for resource caching.

@mahemoff
Copy link
Author

mahemoff commented Mar 2, 2012

AppCache unfortunately has a number of pain points at present. I think
everyone agrees saving scripts in localStorage is a non-pure hack, but if
you're a developer wanting to make your pages load fast today, your options
are limited. And in practice, it's likely to be just a few % of
localStorage capacity.

On Fri, Mar 2, 2012 at 7:55 PM, Divya Manian <
reply@reply.github.com

wrote:

In my view, this use of localstorage is not conducive, and efforts should
be made to make the appCache better as it is dedicated for caching. I
assert localStorage is best used with data (e.g. to-do lists, map points,
svg paths(?)) not for resource caching.


Reply to this email directly or view it on GitHub:
#1 (comment)

  • Sent from my web browser

@addyosmani
Copy link
Owner

I agree that a more succinct API would be preferable here. Perhaps:

  • require()
  • add() (for merely adding to localStorage if not present, with perhaps an optional switch to overwrite if it is)
  • remove()
  • wait()

would cover all our needs.

In terms of @nimbupani's comments regarding localStorage usage here, I have to say that I'm personally still on the fence about this approach. I've read Google, Bing and Facebook engineers say that there are definitely performance benefits to storing scripts in this manner (despite localStorage not (at least in my view) having been created for this purpose). What I would like this project to do is:

  • Give developers a reference point if they do want to add localStorage script caching to their app
  • Provide public benchmarks that demonstrate whether there is any validity to using localStorage for this at all. To date, I still haven't personally seen any beyond what's been told to me and I think its needed if this idea is to have legs.

+1 to @mahemoff. This is really just a hack until the alternatives improve.

@addyosmani
Copy link
Owner

I've revised the API to meet the above in the latest version. I'll be tweaking this further to cover a few more capabilities soon, but closing this issue for now.

GerHobbelt pushed a commit to GerHobbelt/basket.js that referenced this issue Mar 8, 2022
GerHobbelt pushed a commit to GerHobbelt/basket.js that referenced this issue Mar 8, 2022
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

No branches or pull requests

4 participants