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
base: minor
from

Conversation

Projects
None yet
4 participants
@moschel
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Jun 3, 2014

Contributor

@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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@DVSoftware

DVSoftware Jun 3, 2014

Contributor

@moschel surround it with

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

DVSoftware commented Jun 3, 2014

@moschel surround it with

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jun 3, 2014

Contributor

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

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

Show outdated Hide outdated map/map.js
@@ -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 (){}") {

This comment has been minimized.

@justinbmeyer

justinbmeyer Jun 3, 2014

Contributor

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

justinbmeyer Jun 3, 2014

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 3, 2014

Contributor

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

Contributor

justinbmeyer commented Jun 3, 2014

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

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Jun 3, 2014

Contributor

@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?

Contributor

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 referenced this pull request Jun 5, 2014

Merged

Warnings for map/define #1054

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Jun 5, 2014

Contributor

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

Contributor

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 Nov 24, 2014

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