Re: [GSoC][PATCH v2] Add a diff driver for JavaScript languages.

From: Glen Choo <chooglen@google.com>
Date: Mar 15, 2022, 21:34 UTC
Message-ID: <kl6l8rtayk5o.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq1qz3dnqu.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 13 quoted lines

> Glen Choo <chooglen@google.com> writes:
>
>> Interesting, I'll take note. I'm still personally not keen on supporting
>> CommonJS-only patterns when we are purportedly trying to show diffs for
>> JavaScript, but if we think this fits the style, I'm happy to oblige.
>
> The question is, with these patterns that are aware of CommonJS
> convention, would your bog-standard-and-boring vanilla JS code be
> detected incorrectly?  Becoming aware of popular conventions without
> hurting others would be a good thing.
>
> And the "popular conventions" does not have to be limited to
> CommonJS/Node.

From the perspective of "'exports' is a special name", yes, we could detect vanilla JS code 'incorrectly' because, in vanilla JS, the names 'exports' or 'module.exports' are not special. So perhaps, one could imagine a browser-side script that deals with "imports" and "exports" as part of their business:

  const exports = {
    quantity: 1,
    type: 'boxes',
  };
  exports.getQuantity = () => {
    foo();
  };

This diff driver would mistakenly detect `exports.getQuantity = () => {`.

Although, the more I think about it, the spirit of this patch seems to be "we want to show headers whenever we think we are in a function", so we don't actually need to treat 'exports' or 'module.exports' specially at all, e.g. this case should also pass our diff driver tests:

  const foo = {};
  foo.RIGHT = () => {

    ChangeMe();
  };

and if we do this, we will correctly handle 'exports' and 'module.exports' anyway by virtue of them being plain old JS objects.