non_constant_identifier_names should ignore externs

[nex3]

The non_constant_identifier_names lint complains about names that don't use lower camel-case, even if those names comes from JS externs and are thus both not subject to Dart style rules, and generally impossible to rename. For example:

import 'package:js/js.dart';

@JS()
class Exports {
  external set Exception(Function function);
}

produces:

void main() {
  print('hello');
}

[lrhn]

Being external isn't by itself sufficient to ignore the style guide, but being bound directly to JavaScript members of the same name probably is. So, only ignore external declarations if they are bound by a JS annotation or something similar and recognized which gives a reason for the name not following Dart naming rules.

[scheglov]

@pq

[pq]

@lrhn's proposal sounds reasonable to me.

(/cc @natebosch, @jakemac53 for possible opinions.)

[natebosch]

It would be better if we supported renames for @JS() members but I don't think that is getting active work. https://github.com/dart-lang/sdk/issues/39056

I agree that the lint should allow these cases if we can. We should check with @sigmundch and @srujzs to make sure we aren't depending on implementation details that aren't stable.

[sigmundch]

Note that only instance members cannot be renamed, so the rule would still be applicable to static top-level methods.

I think it's OK to bypass the lint on JS-interop external instance members. On the web the only thing allowed to be external are JS-interop APIs and our compilers already enforce that those are bound by a JS annotation (e.g. in the enclosing class), so I'd be OK with a simpler rule for the linter if that helps make the implementation simpler. For example, we could exclude external instance members if the enclosing library signals that it is a web library (e.g. imports package:js or dart:html or dart:js). Note: we used to require that any JS-interop code was in a library that contains a @JS at the library declaration. We removed that requirement from the compilers, so we shouldn't depend on that for this lint. In fact, I'll file a new bug to deprecate another lint which suggests adding that annotation on the library declaration.

As for support of renames in JSinterop - we will not add support for renames of instance methods. However, we will allow them on static extension methods (and eventually on extension types). This is the equivalent of sealed members as we were discussing them long ago and in the bug @natebosch refers to.

The changes to support this have landed recently, so I expect in Dart 2.15 you will be able to write:

@JS()
class Exports {}

extension ExportExtensions on Exports {
  @JS('Exception')
  external set exception(Function f);  // OK to apply the lint on this extension member
}

Given that static extension methods will support renames, I'd be OK enforcing the lint on those members too (so we continue to only exclude actual instance members).

[jodinathan]

This seems nice! There are a lot of JS properties that need renaming.

@sigmundch if I understood it correctly the best approach for js interop would be declare the class with @JS() and do the rest in the extension?

[sigmundch]

Precisely, the class declaration would only have a constructor but no members beyond that. I can confirm now that 2.15 indeed has all the changes needed to support that pattern.

We hope that in 2.16 there will be an additional annotation available that, if use on @JS classes that follow this pattern, then you'll be able to use them on types that are also exposed on dart:html.

[jodinathan]

very good :D

what about static properties?
for example, in gojs we have:

@JS()
class AnimationManager {
  @JS('None')
  external static AnimationManager get none;
}

is this ok in 2.15?

[sigmundch]

I believe so - but I thought that was also working prior to 2.15. Were you running into trouble with that in the past?

[jodinathan]

I believe so - but I thought that was also working prior to 2.15. Were you running into trouble with that in the past?

This works. Some dev reported it wasn't renaming but was his mistake. Sorry about that.

We tried to rename some @anonymous constructor argument:

external factory Foo({@JS('Bar') String bar});

But didn't work. Can it be renamed somehow?

[sigmundch]

we don't have support for that at the moment. The closest we have to that would be to use jsify (similary callConstructor for non-anonymous classes). Note however that jsify is more expensive than the external factory declaration, but if you find this need to be occurring a lot, we can look into making it less expensive in the future (e.g. if we can identify that the argument to jsify is a map literal with simple keys and values)

[jodinathan]

For now that is not important as it doesn't stop us from anything. Just semantics to make code inline with Dart style.
Although I think it may be a problem if we get something like prop.prop2 but I am not sure.
Thanks for all the info so far.