Comments (19)
@amovah getter is a ECMAScript 5.1 feature, and since it isn't supported in all browsers, it seems to me like it would defeat the point of a feature detection library.
from feature.js.
@amovah Iām with @jcxplorer on this one
from feature.js.
What about memoizing each of the checks? I realize you only want to compute the check once, but as the list grows, so does the computation happening on load.
Then the API would be:
feature.webGL(); // returns computed value
feature.webGL(); // returns cached value
from feature.js.
@dictions Sounds like a good idea to me, but it'll be a breaking API change. @viljamis what do you think?
from feature.js.
One advantage to the current API is that it's less error-prone:
if (feature.webGL) { // Correct
if (feature.webGL()) { // Throws exception
Compared to:
if (feature.webGL) { // Always evaluates to true
if (feature.webGL()) { // Correct
Not saying it'd be a bad change, but something to consider nonetheless.
from feature.js.
If we use function, it improves the speed. in other word, change all properties to function (not auto-run function) to prevent load all of them in load time, and it would be executed when we call them.
memoizing it's good idea, but not necessary. anyway I think we must change the API.
@viljamis what you think about?
from feature.js.
How about this then:
// code to detect whether getters are supported
var getterSupported = ...
// all checks as functions
function webGLSupported() {
// ...
return result;
}
// others...
if (getterSupported) {
// all features as smart/memoized/cached getters
feature = {
get webGL() {
delete this.webGL;
this.webGL = webGLSupported();
},
// others...
};
} else {
// all features as properties (no improvement here)
feature.webGL = webGLSupported();
// others...
}
// add the new "getter" feature (already checked)
feature.getter = getterSupported;
This might slightly improve the runtime performance at the cost of increasing the load time a bit (size). I guess the safest and fastest way to do it is to use functions instead of properties.
from feature.js.
@aaalsaleh interesting, I kind of like that solution. The way you're doing if/else is creating a lot of duplication. What about a loop?
// code to detect whether getters are supported
var getterSupported = ...
// all checks as functions
var FeatureChecks = {
webGLSupported: function() {
// ...
return result;
},
// ...
}
var features = {};
// Convert to getters or run all checks
if (getterSupported) {
for (var check in FeatureChecks) {
Object.defineProperty(features.prototype, check, {get: FeatureChecks[check]});
}
} else {
for (var check in FeatureChecks) {
features[check] = FeatureChecks[check]();
}
}
return features;
from feature.js.
In my opinion, It's not a good idea, both of them.
@aaalsaleh opinion: if we check getter support, then we should have duplicated code and two API. some browsers didn't support getter, and API will be changed to function, and some of them support, and it will be changed to properties. people doesn't know which them is true and what they should use right now.
@dictions opinion: our purpose to use getter or function to improve load time. re-define properties, decrease load time, and again we have two different API.
from feature.js.
@amovah I think you misunderstood javascript getter feature. We will have only one API regardless of browser support: properties. However one of them will only be calculated once accessed and cached (memoized getter), while the other will calculate all of them at load time. With the suggestion of @dictions the redefinition lines is kept around 10 lines only.
@dictions Even better. However, keep in mind that your suggestion will not be memoized/cached getter like the one I suggested, but it will be lazy getter nonetheless. Regardless, here is small modifications:
// code to detect whether getters are supported
var getterSupported = ...
// all checks as functions
var FeatureChecks = {
// without Supported
webGL: function() {
// ...
return result;
},
// ...
}
var features = {};
// Convert to getters or run all checks
if (getterSupported) {
for (var check in FeatureChecks) {
if (FeatureChecks.hasOwnProperty(check)) {
Object.defineProperty(features.prototype, check, {get: FeatureChecks[check]});
}
}
} else {
for (var check in FeatureChecks) {
if (FeatureChecks.hasOwnProperty(check)) {
features[check] = FeatureChecks[check]();
}
}
}
return features;
I still believe it is a tradeoff, and more testing is needed to determine the performance gains.
from feature.js.
@aaalsaleh as I told, Memorizing it's good idea, but not necessary. now we need a better api. and you misunderstood javascript. because your code is wrong and features.prototype is not an Object. and memorizing, first we should fix load time, and then we will do it too.
anyway, Yes, I'm sorry. I didn't read whole of the code. I spoke about that algorithm.
It's good idea. but I've created this issue to prevent load all of properties in load time. but if the browser doesn't support getter, we wil go to load all of them, see below:
...
else {
for (var check in FeatureChecks) {
if (FeatureChecks.hasOwnProperty(check)) {
features[check] = FeatureChecks[check]();
}
}
}
our purpose is preventing load all of them and not do as we did. Still, I'm thinking function is better.
what do you think about it? am I right?
from feature.js.
@amovah You started the whole getter idea, I was only suggesting a workaround for browsers not supporting the feature ;)
It is either keep the API and use getter when supported, or change the API to functions as @dictions suggested earlier. The latter is a bit better if you don't mind changing the API.
from feature.js.
@aaalsaleh , as I told we must to change the API. and as I told, our purpose of using getter is increasing load time.
if (getterSupported) {
for (var check in FeatureChecks) {
if (FeatureChecks.hasOwnProperty(check)) {
Object.defineProperty(features.prototype, check, {get: FeatureChecks[check]});
}
}
}
until here it might be good idea. but rest of it, it's not good idea. it's against our purpose of using getter or function. it loads whole of checkers in load time.
Conclusion:
if we use getter, some browsers don't support it. if we check that whether browser supports getter, use it, otherwise call functions. PROBLEM is here, calling all of them. it's decreasing load time. so we can't use getter and we should don't load all of them in load time. so what should we do?
in my opinion, using function instead all of them it's better idea. all browser supports and it doesn't execute all of them in load time.
from feature.js.
IE8 might be no longer supported by Microsoft, but it still exists; Object.defineProperty
will fail if you aren't calling it on a DOM node.
from feature.js.
Interesting point @ShirtlessKirk, though IE8 doesn't support getter/setter, so I don't think it would matter. I guess we could check for both, though now we're adding a lot of extra complexity.
RE functions vs getters: I totally agree with @amovah that the point is to decrease initial load time. That comes at the expense of either changing the API or gracefully degrading to browsers that don't support getters.
My vote is for memoized functions.
from feature.js.
I think memorized functions is not a good idea for this project. because we check a feature for once, and browsers can't mutate then support a feature suddenly.
from feature.js.
If you use functions:
- All browsers support it
- Don't have to check for getter support
- You can pass an object literal for options:
Say:
feature.classList({
toggle: true
}) // true if toggle also exist
Which for example would check if the toggle
method exist?
Obviously that all depends on how you decide to set up the options
param
You could also take advantage of the localStorage
api and store feature support by passing something like:
feature.classList({
toggle: true,
cache: true
});
It can get more complicated and perhaps instead of passing a boolean for toggle
you pass a type:
feature.classList({
toggle: Function,
cache: true
});
To seperate property methods and your the library's own set of properties you could:
feature.classList({
methods: {
toggle: Function
},
cache: true
});
Or just use something like a prefix of $
for flags:
feature.classList({
toggle: Function
$cache: true
});
This would be really ugly for an if statement so perhaps it could return a useful object with methods like exec
:
feature.classList({
toggle: Function
$cache: true
}).exec(successCB, failedCB);
Some real code example (which takes advantage of closure):
var test = feature.classList({
toggle: Function
});
someBtn.addEventListener('click', function(evt) {
test.exec(function() {
node.classList.toggle('example');
}, function() {
if (node.classList.contains('example')) {
node.classList.remove('example');
} else {
node.classList.add('example');
}
});
});
from feature.js.
Hi @viljamis,
what do you think about this?
If you don't want change anything, I will fork and make changes.
from feature.js.
If we can avoid forcing the browser to run each of these methods on page load needlessly, I see that as a win. However it would obviously be unavoidable on the first page load if someone is calling the testAll
method.
There is also this issue which requests modular tests. Perhaps the solution lies in including tests specific to your application's code and not the entire library all at once?
from feature.js.
Related Issues (20)
- websocket support? HOT 1
- No mention of browser support? HOT 3
- v1.0.1 is missing on npm HOT 2
- Would using getters make it faster? HOT 2
- comma or semi-colon? HOT 1
- Check for window for isomorphic JS
- Check for window to support Isomorphic JS HOT 3
- false negative for <img> sizes parameter in Safari HOT 2
- add Cookie enable check pls HOT 3
- IE has touch supported? HOT 2
- Use of util.old contradicts the point of feature detection HOT 1
- <svg>.scrollIintoView not supported on Edge could this be tested? HOT 1
- WebGL check fails on Windows 7 Chrome 55 HOT 2
- Is this repository still maintained? HOT 4
- Gulp warnings on install HOT 1
- Gulp warnings on install
- Update tests/index.html to be clearer
- Access is denied for this document
- Depreciation of navigator.userAgent, navigator.appVersion, and navigator.platform in Chrome HOT 2
- Any plan to support PWA capability detection?
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
š Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ššš
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ā¤ļø Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from feature.js.