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

Substitute util.bind with Function.bind #618

Merged
merged 1 commit into from Jul 27, 2020

Conversation

dilyanpalauzov
Copy link

As of https://caniuse.com/#feat=mdn-javascript_builtins_function_bind Function.bind is supported in IE +9, Edge +12 , Firefox +4, Chrome +7, Safari +5.1.

ScheduleCreationPopup._onClickListeners() uses Function.bind since April 2018, and TimeCreationGuide._onDrag uses it since Mai 2017, so the aforementioned browser versions are required that long.

• Replace util.bind(x,y) with x.bind(y)
• Update README.md with the minimal supported versions.

Please check if the PR fulfills these requirements

  • It's submitted to right branch according to our branching model
  • It's right issue type on title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has description for the breaking change

@seonim-ryu
Copy link
Member

Can one of the admins verify this patch?

README.md Outdated
@@ -238,7 +238,7 @@ $('#calendar').tuiCalendar({
## 🌏 Browser Support
| <img src="https://user-images.githubusercontent.com/1215767/34348387-a2e64588-ea4d-11e7-8267-a43365103afe.png" alt="Chrome" width="16px" height="16px" /> Chrome | <img src="https://user-images.githubusercontent.com/1215767/34348590-250b3ca2-ea4f-11e7-9efb-da953359321f.png" alt="IE" width="16px" height="16px" /> Internet Explorer | <img src="https://user-images.githubusercontent.com/1215767/34348380-93e77ae8-ea4d-11e7-8696-9a989ddbbbf5.png" alt="Edge" width="16px" height="16px" /> Edge | <img src="https://user-images.githubusercontent.com/1215767/34348394-a981f892-ea4d-11e7-9156-d128d58386b9.png" alt="Safari" width="16px" height="16px" /> Safari | <img src="https://user-images.githubusercontent.com/1215767/34348383-9e7ed492-ea4d-11e7-910c-03b39d52f496.png" alt="Firefox" width="16px" height="16px" /> Firefox |
| :---------: | :---------: | :---------: | :---------: | :---------: |
| Yes | +9 | Yes | Yes | Yes |
| +7 | +9 | Yes | +5.1 | +4 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supported browser versions should not be depending on compatibility of Function.prototype.bind

@dilyanpalauzov
Copy link
Author

TimeCreationGuide._onDrag uses Function.bind since 2017 and therefore Safari 5.1+ is required that long. Just the documentation was not updated accordingly.

@jungeun-cho
Copy link
Contributor

I think the browser support of the calendar application should not depend on any particular feature or method.

In the past, we used to support calendars even under IE9. So instead of using the bind function immediately, it was written using utility functions like a code snippet using call and apply.

The set of these utility functions is tui.code-snippet, but in calendar 1.x version, use tui.code-snippet version 1.x.

We are currently developing 2.0, and in 2.0 we will use tui.code-snippet 2.x and Function.prototype.bind. We will focus more on 2.0 development.

@dilyanpalauzov
Copy link
Author

https://github.com/nhn/tui.calendar/blob/master/README.md#-browser-support says: “Safari → Yes. Internet Exporer → +9”. Does this mean,that tui calendar runs on Safari 3?

@dilyanpalauzov
Copy link
Author

Besides, url('image/ic-unlock.png') was included twice in the css files: once in .{css-prefix}icon.{css-prefix}ic-public and once in .{css-prefix}section-private.{css-prefix}public .{css-prefix}ic-private. This resulted including the same uri:data: in dist/tui-calendar.css which bloats that file unnecessary.

@dilyanpalauzov
Copy link
Author

handler/daygrid/creation.js contains domutils.getClass(target).trim(). String.trim() is supported as of https://caniuse.com/#feat=mdn-javascript_builtins_string_trim in Safari 5 but not in Safari 4. So Safari 5 is required since 2018 (since creation.js is that old).

@stale
Copy link

stale bot commented Jul 20, 2020

This issue has been automatically marked as inactive because there hasn’t been much going on it lately. It is going to be closed after 7 days. Thanks!

@stale stale bot added the inactive label Jul 20, 2020
@dilyanpalauzov
Copy link
Author

I would like to appeal once again to review which browsers version can be used to execute the current code, e.g. by checking what functions does the current codebase use.

@stale stale bot removed the inactive label Jul 21, 2020
@jungeun-cho
Copy link
Contributor

jungeun-cho commented Jul 23, 2020

@dilyanpalauzov
If you want to notify the browser version correctly,
Not only bind and trim
Browser compatibility should be checked in all code bases.
(For example, using the browser stack...)

The meaning yes was used as the latest. Normally, modern browsers are automatically updated. How many users use in safari3, safari4 ?

But there are still quite a few people using older IE browsers.

@dilyanpalauzov
Copy link
Author

Nobody knows, what JavaScript functions can be used in the code, that are available in the browsers, and which not. This leads to bloated code.

E.g. currently the code uses Function.bind and according to https://caniuse.com/#feat=mdn-javascript_builtins_function_bind and https://caniuse.com/#search=indexof all browsers, which support Function.bind() also support Array.indexOf(). Yet the codebase uses instead tui-code-snippet.util.array.inArray() which is a (slower) polyfill for Array.indexOf(). This polyfill returns returns -1, when the parameter is not an array, but the polyfill is used also at places, where the parameter is clearly an array. At the same time Array.indexOf() is also used in the codebase, so that browsers running tui.calendar do understand it. Replacing util.inArray() with Array.indexOf() makes the code easier to understand and faster.

To know what can be used in CSS and JavaScritp, and what needs hacks, one has to describe very precisely the supported browsers. Firefox before 4 and Chrome before 7 are not supported.

@jungeun-cho
Copy link
Contributor

It would make more sense to test all browser versions and write the correct version.

The meaning of yes is used with the meaning of latest and supported, and after testing for all browsers, it will not be changed unless it is the correct browser version.

@jungeun-cho
Copy link
Contributor

@dilyanpalauzov
Can you re-request PR only to improve the bind?
can you revert only the README in this PR?

Can you register the browser compatibility part mentioned as a separate issue?
I'm going to discuss this with the team. :)

ScheduleCreationPopup._onClickListeners() uses Function.bind since April 2018, and
TimeCreationGuide._onDrag uses it since Mai 2017, so Function.bind can be used
everywhere and util.bind is not necessary.

• Replace util.bind(x,y) with x.bind(y)
• Update README.md with the minimal supported versions.
@dilyanpalauzov
Copy link
Author

I have changed the herein proposed changes to only substitute util.bind() with Function.bind().

The discussion on documenting the minimal supported browsers in README.md is now at #666.

@dilyanpalauzov dilyanpalauzov changed the title docs+refactor+perf: Lift the required browser versions Substitute util.bind with Function.bind Jul 23, 2020
@jungeun-cho
Copy link
Contributor

ok to test

Copy link
Contributor

@jungeun-cho jungeun-cho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[7/27] I reviewed. :)

@jungeun-cho jungeun-cho merged commit 5adbd7b into nhn:master Jul 27, 2020
@dilyanpalauzov dilyanpalauzov deleted the unbind branch July 27, 2020 06:46
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.

None yet

4 participants