String key changes break extensions

[jasonsanjose]

See #7772

Our extension uses Strings.FILE_ALREADY_EXISTS in an error dialog before overwriting a file. Now that the key is missing, we hit runtime errors instead.

cc @ryanstewart (if you were on Brackets master last week instead of the sprint 39 release, you probably ran into this bug.

Also, none of the translations were updated to use the new string key.

[jasonsanjose]

Assigning @JeffryBooher, high priority breaking API change.

[JeffryBooher]

@jasonsanjose do you want me to re-add this string? It isn't being used by Brackets anymore so it seems like the correct way to fix this would be to add it to your extension's strings.js file.

#7926 & #7893 are updated string translations. I'm not sure what you mean by "none of the translations were updated..."

[jasonsanjose]

Hmm, I just did a search on the repository and found a bunch of translations that still have the FILE_ALREADY_EXISTS key https://github.com/adobe/brackets/search?q=file_already_exists&ref=cmdform ... maybe they just need to be removed?

I think we should be careful about removing strings and we should treat these changes as API changes. We're happy to update our extension based on what you guys decide is the right thing to do.

[JeffryBooher]

@jasonsanjose well those strings aren't used anymore in lieu of the new strings which means they show as english in those languages that have yet to be translated.

I don't have an opinion either way -- i don't know if this is a case that we've run into before. It seems that extensions should really use their own strings. With API changes, we can easily map them to other APIs and display a deprecation warning which means that extension authors are on warning that the API will be removed at some point and they should find a different implementation. With strings and commands, etc... we just have to have a getter that does this but it seems a little overkill. Also I'm not quite certain how this works since i18n overlays the root with the strings. I'm not sure if that means our getter would get called or not since we would put it on the root.

@redmunds any ideas?

[redmunds]

I never really thought about Strings as an API, but marking as Needs Review to get attention of team to see how everyone else feels.

[JeffryBooher]

actually I just talked with @peterflynn and this is pretty standard practice to delete strings like this. The correct way to use strings in extensions is to create a strings.js and localize that as my new project extension creates for the new brackets extension project

[peterflynn]

We've definitely deleted strings before without treating it as an API change -- similar to changing our CSS or minor things in the DOM layout. It seems rare enough for extensions to depend on this that IMHO it's generally ok not to worry about it. @jasonsanjose I think for our use case it probably makes sense just to have a separate strings file in the extension.

Re keeping translations up to date: we could definitely improve on that for community-contributed translations. Part of the new(ish) branching process is that it gives a nicer window of time for people to post translation updates & get them merged in time for the release. And @RaymondLim has done some great work to make it easier to diff what needs to be updated each sprint, which we're just beginning to see the benefits of now...

[jasonsanjose]

Fair enough. Guess I'll close this for now. Maybe more broadly we should have a more unified experience for standard dialogs like in this "file overwrite" case where an extension wants to overwrite an existing file. I would hope that we could agree on showing a standard warning string in this case.

[peterflynn]

@jasonsanjose Ah, you mean like making FileUtils.showFileOpenError() more configurable? That seems reasonable to me...