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

Cache objects in Utils.__cache instead of using $.data (#4014) #4346

Closed
wants to merge 13 commits into from

Conversation

NadeemAfana
Copy link

@NadeemAfana NadeemAfana commented May 6, 2016

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

  • Cache objects in Utils.__cache instead of using $.data
  • Updated several files to use the new functions to cache data in Utils.__cache
  • Removed all $.data code

This is a bug fix for #4014

@kevin-brown
Copy link
Member

There are indentation changes in here that are going to prevent this from being merged in.

@kevin-brown kevin-brown added the 4.x label May 6, 2016
@NadeemAfana
Copy link
Author

I will fix the indentation.

@kevin-brown
Copy link
Member

  • There are still some indentation issues
  • This is going to need integration tests to make sure that we don't have this issue in the future

@NadeemAfana
Copy link
Author

Sorry for the indentation issues. I fixed the EOL to be UNIX format. My editor had switched this to Windows format.

Do you want me to add integration tests ?

@alexweissman
Copy link
Contributor

There's an integration test now...can this be merged in?

@alexweissman alexweissman added this to the 4.0.6 milestone Oct 26, 2017
@alexweissman
Copy link
Contributor

Merged into develop for 4.0.6 release.

@oh-ren
Copy link

oh-ren commented Nov 18, 2017

Please also consider the following discussion regarding $myOriginalSelectElement.data('select2'): #5071 (comment)

@chrisdeeming
Copy link

Sorry for posting here, but it feels relevant and I've tried to get support from the official channels with no answers yet.

Although there’s a fallback in Utils.GetData to get from the data attribute if the cache is empty, there’s nothing that actually sets the data attribute in the first place.

What I’m trying to do is access what was equivalent to:

var instance = $element.data('select2');

But this no longer seems to be possible.

Is there a public way of accessing the GetData or SetData functions?

Is this not a sort of dangerous change to make in essentially a bug fix release in terms of breaking backwards compatibility?

@alexweissman
Copy link
Contributor

I'm here, just been busy with php[world] and traveling.

@chrisdeeming
Copy link

To be fair, I've not exactly been waiting long for a reply (and it's a weekend!), but I didn't necessarily want to see this one slip into a stable release if there's something wrong.

@alexweissman
Copy link
Contributor

Yes, thanks @oh-ren and @chrisdeeming. I didn't realize that these changes affected the public API.

But, it seems like this is meant to fix a bug (#4014). Honestly, I don't really understand the problem that this is solving as I have never encountered this myself. It seemed like a safe change, but apparently not.

Is there a way we can fix this bug (whatever it may be) without causing a breaking change? Join me on my own chat server (https://chat.userfrosting.com/channel/select2) and we can discuss.

@alexweissman alexweissman reopened this Nov 18, 2017
@alexweissman alexweissman changed the title Bug fix for #4014 Cache objects in Utils.__cache instead of using $.data (#4014) Nov 19, 2017
@alexweissman
Copy link
Contributor

@NadeemAfana any thoughts on this? Seems like this is breaking a lot of other aspects of Select2.

@NadeemAfana
Copy link
Author

$element.data('select2') is clearly not public API; it is an internal implementation which people should avoid. Nonetheless, I will push a fix for this.

@alexweissman
Copy link
Contributor

Thanks! Yes, unfortunately Select2 is so big and widely used, and had been poorly documented for so long, and tends to attract the greenest types of developers, that it's part of the public API whether we want it to be or not 😦

Are there any use cases where people really need to be able to access data('select2')? Otherwise we can add something to the documentation and then deprecate it over time.

Also, is there any chance that this modification is what is causing #5133?

@NadeemAfana
Copy link
Author

I would definitely deprecate .data('select2'). It makes things confusing when people start using the internal components as they change over time. For example, updating .data('select2').options.options.maximumSelectionLength directly has no effect. According to this comment, it worked in the past, but this change has nothing to do with this PR.

Also, is there any chance that this modification is what is causing #5133?

No, because I was able to reproduce the issue with 4.0.5.

@chrisdeeming
Copy link

We use the data stored there specifically to access the elements which are dynamically created by Select2, mostly as a shortcut.

They are, specifically .select2-container and .select2-selection.

We could just find them in the DOM relative to element we've instantiated Select2 on but if they're already cached in the Select2 object then, why not?

Point taken about invalid use cases but that's not something we'd do.

@oh-ren
Copy link

oh-ren commented Nov 20, 2017

I suggest to create a new method then, perhaps $mySelect.select2('getInstance')?
To reiterate my use case (there might be others of course) here: add a class to $container when the select is deemed invalid (for styling purposes).

Not sure how other (popular) jQuery plugins implement getting a reference / access to the internal instance. It's not something uncommon, or is it?
Anyway it provides a means to get things done that otherwise are impossible / the plugin doesn't cater for. Manually selecting based on classes (or manual DOM traversal) is like 10000 times worse than being able to access the relevant elements already constructed by Select2 itself.

(Of course, there could be a big fat warning accompanying the method that relying on it is at your own peril 😉)

(Coming to mind, off topic but somewhat related): $mySelect.hasClass("select2-hidden-accessible") (ref) is not that great, why not something like: $mySelect.select2('isInitialized')?)

@alexweissman Thanks for the invite, might check the channel out, but simply discussing here works best?

@chrisdeeming
Copy link

@oh-ren I would indeed say that something similar to $mySelect.select2('getInstance') is generally the norm in other libraries.

That said, the data attribute approach is not uncommon either. As a compromise, if there was a built in method which could get certain things (but not set them) then that would work too. $mySelect.select2('getInstance') could just get the bits that are considered "safe" to expose.

@alexweissman
Copy link
Contributor

Keep in mind also, that modifications to the internals of Select2 should be possible with custom adapters/decorators.

Can you provide examples of other jQuery plugins that have a getInstance method?

Incidentally, this seems like a perfect example of how Javascript's lack of visibility scoping can be frustrating. Perhaps we need a rewrite of Select2 into Typescript more badly than we realize 😁

At any rate, I've merged in @NadeemAfana's BC fix for release on 4.0.6-rc.1, so this should solve the problem for the time being.

@NadeemAfana
Copy link
Author

The tests now run for both jQuery 1.x and 2.x. This allows us to see if there is any discrepancy in the future.

@stale
Copy link

stale bot commented Mar 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@jamessaget
Copy link

Hello,

I have read through issue #5071 which lead me here. What I am trying to achieve is change a config option after initialisation and stumbled into the issue stated here https://stackoverflow.com/questions/37016655/select2-cannot-read-property-query-of-null regarding the error thrown within the dataAdapter.

$('.country_select').on('select2:opening', (e) => {
   if($(e.target).data('select2').options.options.minimumResultsForSearch == 5) return
   $(e.target).data('select2').options.options.minimumResultsForSearch = 5;
   let options = $(e.target).data('select2').options.options;
   $(target).select2('destroy');
   $(target).select2(options);
})

Any suggestions would be appreciated

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

Successfully merging this pull request may close these issues.

None yet

7 participants