Skip to content
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

[CLOSED] Externalize strings from Extension Manager view item template #3271

Open
core-ai-bot opened this issue Aug 29, 2021 · 9 comments
Open

Comments

@core-ai-bot
Copy link
Member

Issue by njx
Friday Apr 19, 2013 at 00:37 GMT
Originally opened as adobe/brackets#3483


Note that the template references the strings as {{Strings.STRING_KEY}} instead of just {{STRING_KEY}}. This is because each item is rendered separately, so it seemed worth it to avoid copying all the strings into the context object for each item, as opposed to just pointing to the entire Strings object in a subfield of the context.


njx included the following code: https://github.com/adobe/brackets/pull/3483/commits

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Apr 19, 2013 at 00:44 GMT


Also, note that I changed "by" and "on" to "Author:" and "Date:". I figured that the new design might separate these various fields out, so having them be prepositions wasn't necessarily going to make sense; also, due to word order differences in foreign languages, it was risky to rely on prepositions to express these relationships (especially since they aren't all coming out of a master string format that can reorder things).

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Friday Apr 19, 2013 at 03:07 GMT


Assigned to me

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Friday Apr 19, 2013 at 04:54 GMT


Mostly minor comments. Could probably merge as-is, but I'll wait until tomorrow in case I don't hear from@njx.

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Apr 19, 2013 at 07:04 GMT


Fixed the version checking to indicate whether a newer or older version of Brackets is needed. (It wasn't completely straightforward because semver doesn't directly tell you whether an unsatisfied range is newer or older than the version you have, so I had to parse the range in order to figure that out.)

While testing this I also noticed that I wasn't actually properly forcing the Extension Manager to retrieve the registry every time you open the dialog (which we want to do)--both because I wasn't actually passing the parameter to tell it to do so, and because passing {cache: true} to $.getJSON() doesn't actually work (its second parameter isn't actually an options param), so I snuck in a fix for that as well. Fixing this required me to fix the unit tests, which were actually assuming that the view creation didn't force a new fetch of the registry (and was therefore expecting it to be synchronous).

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Apr 19, 2013 at 07:05 GMT


@dangoor - you might want to take a look at c465213 to see if the way I'm dealing with semver to determine whether we need an older or newer version of Brackets makes sense here.

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Apr 19, 2013 at 07:10 GMT


Oh, I also changed "Brackets" to {APP_NAME} in the strings in c465213 as well. The caching fixes are in 0f11a33. Ready for re-review.

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Friday Apr 19, 2013 at 13:51 GMT


I'm a little conflicted about c465213 for a couple of reasons.

  1. semver (the module, not the standard) supports another version requirement style: "0.20.x".
  2. it also supports more complex requirements: ">0.20.0 <0.23.0"

In general, I think that doing any parsing of version requirements outside of the semver module is asking for trouble.

The other thing I thought of with respect to this change is that we have the ability to actually offer earlier versions of an extension that may be compatible. If you look at the registry data, you'll see things like this:

"versions":[{"version":"1.0.2","published":"2013-04-10T18:21:27.058Z","brackets":">0.20.0"}]

For each version of an extension, we keep track of Brackets compatibility. So, it's possible that the most recent version is not compatible, but the user could install the previous version. That is a feature that can be added later, though. I just mention it because checking entry.versions[entry.version.length-1].brackets is a better way to go than entry.metadata.engines.brackets because it will hint at the future capability and is also a clean place for us to tweak things in the registry if we detect an incompatibility.

All of that said, I think the code here covers the common cases and won't blow up in the uncommon ones. I think the UX of hinting that a newer version of Brackets or an update to the extension is needed is better than just saying "It doesn't work" and is probably worthwhile.

For the purposes of string freeze, I think it's fine to take this code. But, I think that we should ultimately:

  1. use entry.versions[entry.version.length-1].brackets
  2. get something into semver that does the comparison we want (a version of satisfies that hints at the problem when it doesn't satisfy, or maybe there's some way to use maxSatisfying?)

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Apr 19, 2013 at 16:03 GMT


@dangoor--agreed on both points. Let's merge this as-is today--could you file a bug for the other issues to track them? I think we should fix the first one this sprint--not as sure about the semver change.

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Friday Apr 19, 2013 at 22:00 GMT


Looks good. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant