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

Map define warn #1041

Closed
wants to merge 4 commits into from
Closed

Map define warn #1041

wants to merge 4 commits into from

Conversation

moschel
Copy link
Contributor

@moschel moschel commented Jun 3, 2014

related to https://forum.javascriptmvc.com/topic/can-route-map-working-with-arrays

I've seen a few times that people forget to add the define plugin and it fails silently. This is an attempt to add a warning so its obvious right away.

@moschel
Copy link
Contributor Author

moschel commented Jun 3, 2014

@daffl this is failing jshint:

[L248:C17] W098: 'MapClass' is defined but never used.
    var MapClass = can.Map.extend({

In this case, that's intended. How can I ignore that for this test?

@DVSoftware
Copy link
Contributor

@moschel surround it with

/* jshint ignore:start */
your code
/* jshint ignore:end */

@daffl
Copy link
Contributor

daffl commented Jun 3, 2014

Why not take it out? The test should still do what it is supposed to without the var MapClass.

@daffl daffl added this to the 2.1.2 milestone Jun 3, 2014
@@ -49,6 +49,12 @@ steal('can/util', 'can/util/bind','./bubble.js', 'can/construct', 'can/util/batc
this._computes = [];

for (var prop in this.prototype) {
//!steal-remove-start
if(prop === "define" && this.helpers.define.toString() === "function (){}") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you check if prop === define on every iteration of this loop? Can you you just check if "prop" in this.prototype outside this loop.

Also, using .toString() isn't supported everywhere ... is this the best way of checking if the define plugin is loaded?

@justinbmeyer
Copy link
Contributor

I'm not sure we should be doing this. There are lots of plugins and we don't warn about each one.

@moschel
Copy link
Contributor Author

moschel commented Jun 3, 2014

@daffl that test wouldn't work without a map definition

@justinbmeyer lately we've been adding warnings for "silent failure" type things, like wrong property names in a mustache template. Most plugins don't fail silently, they define a function that gets called and user code throws an error without it, or something similar. This fails silently.

I added a better check, the problem was there is nothing special defined in the define plugin that could be inspected for feature detection code, so I changed the function stub. Do you see any reason this could be bad?

@moschel moschel mentioned this pull request Jun 5, 2014
@moschel
Copy link
Contributor Author

moschel commented Jun 5, 2014

did this off of minor, so closing it and opening another

@moschel moschel closed this Jun 5, 2014
@daffl daffl deleted the map-define-warn branch November 24, 2014 18:11
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