Skip to content
This repository

@extends fails for local symbols when file is a @module #101

Open
blixt opened this Issue March 21, 2012 · 11 comments

4 participants

Blixt Jeff Williams Michael Mathews Tim Schaub
Blixt
blixt commented March 21, 2012

When a file is a module, by using the @module tag, @extends stops working for local symbols. Should one really have to specify @extends {module:test~BaseClass}?

See the following test case:

/**
 * @fileoverview This is a test case.
 * @module test
 */


/**
 * This is a base class.
 * @constructor
 */
function BaseClass(x) {
  this.x = x;
}

/**
 * Gets X.
 * @return {number} The value of X.
 */
BaseClass.prototype.getX = function() {
  return this.x;
};


/**
 * This is a class that inherits from the base class.
 * @constructor
 * @extends {BaseClass}
 */
function ChildClass(x, y) {
  BaseClass.call(this, x);
  this.y = y;
}
ChildClass.prototype = Object.create(BaseClass.prototype);
ChildClass.prototype.constructor = ChildClass;

/**
 * Returns a copy of this ChildClass.
 * @return {module:test~ChildClass} The copy.
 */
ChildClass.prototype.copy = function() {
  return new ChildClass(this.x, this.y);
};
Blixt

Ran into this issue again, this time on @memberof and @member declarations. Would it be possible to fix this? It's just not sane to have to specify the module prefix for every declaration inside the very file where the module prefix is defined.

Michael Mathews
Owner

Hello,

Firstly, I don't agree that this is necessarily a bug to fix. It is already possible to document your code example, it's just not as convenient as you think it should be. So I believe the "feature" tag is correct in this case.

Secondly, implementing this feature would raise some questions that need to be answered satisfactorily. For example, in order to not be surprising with our own API I would think this feature would have to be applied consistently. As an example...

/**
 * @fileoverview This is a test case.
 * @module test
 */

/**
 * This is a base class.
 * @constructor
 */
function BaseClass() {
}

/**
 * This is a class is similar to {@link BaseClass}.
 * @constructor
 * @see {BaseClass}
 * @param {BaseClass} b
 */
function OtherClass(b) {
    this.b = b;
}

This example shows three tags that refer to a symbol named "BaseClass". Assume for a moment that there is a global symbol also named "BaseClass" (or whatever name we could choose for this example). In such a scenario it wouldn't be clear which BaseClass was meant whenever the doc referred to a {BaseClass}. As currently implemented the assumption would be that "BaseClass" means the global symbol named "BaseClass". This is unsurprising, because that's exactly how your users would refer to a global BaseClass in their own code, by writing new BaseClass() for example. But, if we were to change things to work as you are asking, there would be an assumption that "BaseClass" means the local one. So how would the code author then write their docs to correct that assumption, if they actually meant the global one?

One answer is to add a new requirement to disambiguate the two with a syntax like @see {global~BaseClass}. But now we have the opposite problem to the one which you've presented, "Should one really have to specify @extends {global~BaseClass}?" Keep in mind that we cannot even safely limit this problem-case to only the times when there is a known ambiguity, because it might be that the global "BaseClass" will be documented later, so we'd have to know when there is an ambiguity and even predict the future to know when there could possibly ever be an ambiguity. This is not currently possible as the JSDoc parser works.

So, it seems to me, we have to fall on one side of this fence or the other: either we assume innermost or outermost "BaseClass" consistently. The decision we make should be based on factors such as: which of the two is least surprising? which is more backwards compatible? which is a more common use case?

In every one of these factors I think the answer is going to go against your proposal, at least partly because it's probably less common to want to produce API docs for an inner class that cannot ever be accessed publicly, whereas it's probably expected that if you refer to a {BaseClass} you mean the thing that is called simply "BaseClass" in the global scope; and this is already the current behaviour, so that makes it 100% expected to anyone who's ever used it already.

Finally, aside from the points above, I don't believe the harm actually caused to the code author, even in your example use case, is all that significant. If writing the "module:test~" many times is onerous, one could just write the docs without it and then do a find and replace in the editor afterwards. If you mean that it's "conceptually" onerous to have to think "module:test~BaseClass" when you prefer to just think "BaseClass" then I refer back to my previous points, that you would merely shift this conceptual burden to a different use case, for the author using a global BaseClass, and that use case deserves the burden less so, in my opinion.

You could step over all these thorns by proposing some completely different syntax, and one that could be implemented as a plugin as well. Consider:

/**
 * @fileoverview This is a test case.
 * @module test
 * @local BaseClass
 */

This new tag would signal that all references to "BaseClass" in this file mean the one local to this module.

I believe it would be possible to write such a plugin, that only for the scope of a given file, would literally replace all references to "{BaseClass" with "{module:test~BaseClass". I'd be much more inclined to accept a feature request like that, because it requires an explicit opt-in from the code author. And I'd be even more convinced by seeing such a feature proven to be usable as a plugin in the wild. I've coded up a quick example to get you started, if you wish to take the plugin route: https://gist.github.com/61faa72a8793d15605ee

Blixt

As much as I like pragmatic approaches, I have to disagree with your concerns about accidentally shadowing global variables. You're right in principle with your points, but in reality you're catering for a very limited corner case.

Using the @module tag should imply that you're in a namespaced file, and that the global namespace is not to depend on. So if you @extend {BaseClass} and that is not in this module, even your code will have an undefined behavior, so correct documentation or not won't help you. You can add a global prefix in addition to the module:x one, if that helps cover these cases.

Also, looking at real-world examples, even Google's huge Closure library does not suffer from what you suggest. It's up to the developer to not write code that shadows globals. And the fact of the matter is that you'll be writing in one of two environments:

  1. For a browser in which case you cannot rely safely on any global outside your module, because who knows what else is included
  2. For an environment that implements CommonJS or similar, in which case your module will be safely scoped and there are no globals to consider (and you may not rely on any externals unless they're explicitly imported)

Now, let's look at the downsides of forcing the maintainer of a module to reference their own module for every single tag and symbol within it, starting with the two you already mentioned:

  1. It's repetitive to write the prefix everywhere
  2. It adds noise to reading the JSDoc notes in the code
  3. Changing the name of the module means updating every reference within the file
  4. And the most important point: developers are lazy and follow intuition – having to use the prefix everywhere is neither easy or intuitive

I don't know how you use JSDoc, but I work at @Spotify and we have a great amount of developers working on lots of different JavaScript modules. I'm not raising this because of a fancy, but because this is a real problem and even though I personally try to follow your scheme, we just have too many developers to get everyone to do this search/replace solution you suggested. Besides, search/replace is the worst solution to a problem when using a tool. I mean I could theoretically use search/replace to replace JavaScript code and comments with HTML describing the file, instead of using JSDoc. But I don't since JSDoc makes the process smoother. And it should for this too.

Again, I urge you to look at real-world examples. Compare how often someone runs into this (every single time they introduce the @module tag to their file) compared to how often they run into the corner case of shadowing a global. We're currently at a crossroads of removing @module everywhere because it makes maintenance too difficult.

Michael Mathews
Owner

You are arguing that I am wrong about my assumptions regarding the implied namespacing within modules by citing the @extend {BaseClass} example, but it weakens your case that you are arguing against an example I never used, and yet you are ignoring the three examples I did: @see, @param, and @link. If you want to make the argument that @extends and @memberof should behave differently to all the other tags in this regard, go ahead. I would just repeat what I already said about being consistent and unsurprising.

Next, I don't see why you are mentioning code authors writing or not writing code that shadow globals. It is syntactically correct code to have two symbols with different scopes and the same name, and JSDoc works with such code, why shouldn't it? I wasn't arguing about your very specific use case: my point was that for more than a decade JSDoc has had a well-defined concept of a symbol namepath like "BaseClass" and it has always meant the global BaseClass. To change that should require a very convincing reason. I'm not saying you haven't provided one (read on).

I don't know how you use JSDoc, but I work at @spotify and we have a great amount of developers working on lots of different JavaScript modules.

Thanks for asking. I work as a Senior Web Developer at the BBC. We also have a few developers who work on lots of JavaScript for our website as well. As the creator of JSDoc I dare say that the way I use JSDoc is: more than most. If your goal is to win a pissing contest with me, that's fine. If you want to convince me of your case, skip the implied condescension in remarks like that, it does nothing to strengthen the merits of your argument.

Ignoring that, you are persuasive when you say that it is an exceptional circumstance within a module, and changing the rules there is thus justified, and I'm certainly not considering closing this ticket out-of-hand. But I can say that the work involved in implementing such a feature would be non-trivial, and as unfanciful as you say this issue is to you and your company (and I sincerely believe you) it doesn't appear to be high on many other user's list. JSDoc is an open source project hosted on an easy-to-fork repo, so you don't need to convince anyone if you badly want this feature, you can just do the necessary work yourself this very day. If that's not of interest then you'll have to wait for a volunteer who is willing to do that work and decides to donate your feature for you. That certainly could happen, after all I already wrote you a plugin for free that demonstrates one way to solve your problem, but if you're really at a crossroads I'd suggest you take the initiative. If you eventually want to merge your new feature back I would absolutely accept it as long as it is backwards compatible, with the exception of the @module case as you've already argued convincingly can be an exception. Either that or I'd be interested in seeing the find-and-replace pattern you have in mind (and there are obviously plenty of other open source alternatives to JSDoc, we'll miss you but no hard feelings).

Blixt

First of all, let's clear the air here.

Thanks for asking. I work as a Senior Web Developer at the BBC. We also have a few developers who work on lots of JavaScript for our website as well. As the creator of JSDoc I dare say that the way I use JSDoc is: more than most. If your goal is to win a pissing contest with me, that's fine. If you want to convince me of your case, skip the implied condescension in remarks like that, it does nothing to strengthen the merits of your argument.

I'm sorry, I must've written my response in a way that seemed offensive. I honestly meant, "I don't know [in what shape or form] you use it" as in that you may have a different use case than we do. Then I provided our use case as a grounds for why this is a legitimate issue, since we're a developer company and we have plenty of developers. It was in no way a "pissing contest" and my intent is to resolve this in an objective (but practical) manner. Please don't get upset, that is not what I'm after. It was casually written and I apologize for the misunderstanding. (I also apologize for the additional comment from "spotify", that was an unfortunate chain of events involving 1. the org having a mailing list e-mail 2. someone hitting reply all and 3. there seemingly being a bug in GitHub where you can't normally post issues as an organization, except from e-mails, which means there is no UI to delete a comment posted by an organization...)

Back to business...

You've got a point about @see, @param and @link, although my perspective doesn't change much. Either you're using modules in which case any of the aforementioned tags should either refer to local symbols, or they should (preferably) refer to module-specific symbols (@see {module:misc~Util}). If you do indeed intend to refer to something you know is global, you could use @see {global~Date}. I would argue that the very use case for @module is to take a step away from references to global symbols. I'm impartial to what happens if there is no @module statement (and indeed, no @module statement at the top has worked as we expected it to).

I understand you want to cover 100% of all code out there, but my argument is that if you can cover 99% of the code out there (the other 1% probably won't use JSDoc), that should be good enough. I would assume that a prerequisite to documenting your code would be that you have fairly structured code in the first place.

I honestly have tried to think of legitimate real-world cases where what you mention will actually be an issue, but for me it's just a theoretical possibility that will probably not happen. Have you encountered this at BBC? Do you think JSDoc would break for anyone if this change was made? If you can easily say yes to those questions, then I retract my assumptions and will consider alternate solutions.

Finally, I'm obviously writing this because me and my fellow developers really like and appreciate your JSDoc library, and the only reason I'm passionate about this is just because we really don't want to have to look for other alternatives. As I mentioned, we're at a crossroad, and the other path is indeed to fork this and implement our own solution. Obviously that is undesirable because of the additional delay/maintenance cost of rebasing etc. We're also aware of the rules of open source and this is a humble request to you, and we do intend to contribute back to the repository if we make changes ourselves.

Blixt

To clarify about my @see example, I meant that if you're in @module mylib, and you want to refer the Util class in the "misc" module, that's when you would write @see {module:misc~Util}. If the Util class was in the same module, you would write @see {Util} and if you're sure that it's in the global namespace (I don't make those assumptions, but anyway), you would write @see {global~Util}.

I'm entirely on the same page as you when it comes to consistency. Again, developers are lazy and intuitive; break the pattern and they will do the wrong thing because it seemed the most intuitive thing to do. :)

Tim Schaub
Collaborator

I think it's fair to say that if you are using modules, you aren't going to be documenting (or linking to) globals.

Michael Mathews
Owner

@tschaub I don't accept that that is safe to say, even if it might be fair. Many client side libs and apps create a global namespace which can be documented. If those libs also use modules, the problem-case is possible. It took me a few minutes to contrive a rather silly example but I don't want to get bogged down in subjective debates about whether globals are likely to be used or a good idea or not. My point was about backwards-compatibility and consistency of JSDoc's API.

I've already conceded the point that @module is exceptional enough that it's probably reasonable to risk any backwards-compatibility in that one case. But I still think this will lead to gotchas and questions about why @module is a special case, and say @event, is not.

@blixt I personally avoid writing classical-looking JavaScript. It's just my preference, so I can safely say I've never encountered your use case, where an inner class, defined in a module scope, is extended with a subclass, and that inner subclass needed to appear in the API docs so frequently that I wanted to avoid writing "BaseClass~". I take your point however that in an "Enterprise Environment", particularly if you have Java developers lurking around, this sort of thing is can be more likely to happen. The nice thing is that JSDoc supports it. At the end of the day you are asking for an optimisation to make an already possible thing easier. Strikes against it: it's not backwards compatible (though I accept that the counter case is probably rare), it creates an inconsistent API, your use case is arguably rare in the general (but I have no data to base that belief on), it's likely to be a lot of work to implement properly, there's a more backwards compatible and consistent halfway solution available with the plugin approach.

Even considering all that, I recognise that those are not rocksolid deal-breakers, and if a good looking pull-request came along with this feature, I would be happy to merge it.

Michael Mathews
Owner

@tschaub just to emphasise, the plugin is my recommendation, and if I were going to implement this feature, even in the core of JSDoc, I would still prefer to use the @local tag. If you really are in a bind and need a fast solution that gets you most of the way there, this seems like the way to go to me. I'd be happy to help with any tweaks or fixes to that plugin, by the way.

Jeff Williams
Owner

I'm planning to resolve this by folding @micmath's plugin into the JSDoc distribution, ideally for the 3.2 release.

Jeff Williams
Owner

Some feedback from @guillaume-brossard-adsk on the current version of the plugin:

Tried the plugin and I didn't succeeded. I'm really not an expert of JSDoc plugins, but 2 things that seems weird:

1- jsdocCommentFound is called to replace local with module:module~local, but it doesn't stick in the generated HTML. [Note: This is almost certainly caused by #228.]

2- newDoclet seems to correctly replace the findCallback namepath by module:module~findCallback, links are correctly sets, but the callback appears in the global.html file...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.