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

ember new --lang #635

Merged
merged 52 commits into from
Jun 12, 2020
Merged

ember new --lang #635

merged 52 commits into from
Jun 12, 2020

Conversation

hergaiety
Copy link
Contributor

@hergaiety hergaiety commented May 29, 2020

Ava Wroten and others added 30 commits April 29, 2020 10:26
Co-authored-by: Jamie White <jameswhite@salesforce.com>
Co-authored-by: Jamie White <jameswhite@salesforce.com>
Co-authored-by: Jamie White <jameswhite@salesforce.com>
Co-authored-by: Melanie Sumner <melaniersumner@gmail.com>
Co-authored-by: Melanie Sumner <melaniersumner@gmail.com>
@Gaurav0
Copy link
Contributor

Gaurav0 commented May 29, 2020

What happens if no --lang flag is passed?

Can a default lang setting be set in ~/.ember-cli or ~/.ember-cli.js?

Unrecognized language subtag, "uk".
```

#### No argument passed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this is just a "bug" / limitation of the command line argument parser used by Ember CLI. This RFC introduces a new named argument that takes a string value. If you didn't pass a value, it should be an error. If a second named argument follows it, it should not be parsed as the value. Basically it should work the same as --blueprint or anything else like it.

I think it's fine to implement whatever workaround is needed, but I'm not sure it needs to be included in the RFC. It's fine to include it for additional information, I suppose, but it shouldn't be considered "normative". Once we fix any bugs or limitations in the arguments parsing, this section will become obsolete.

TL;DR I don't think the RFC should be or is propose a permeant special error handling of this particular CLI argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to implement whatever workaround is needed, but I'm not sure it needs to be included in the RFC.

Ya, agreed (RE: this being a bug).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chancancode @rwjblue I'm sure we're willing to incorporate feedback for what in ember-cli is a bug and what's intended to be there- I mostly assume that the things that are there are intended to be there unless there are code comments that indicate otherwise.

See `ember <command> help` for more information.
```

#### Common Misunderstandings: Programming Languages
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is just a special case of the "Invalid Language Codes". In implementation speak:

let lang;

try {
  // validateLangFlag checks against the ISO 3166 list
  lang = validateLangFlag(langFlag);
} catch(e) {
  if (isProgrammingLanguage(langFlag)) {
    throw new Error("specific error message about how this flag is not doing what you think");
  } else {
    throw new Error("generic error message about how this is not a valid language");
  }
}

In the unlikely case that they overlap (that it is both a valid language and a programming language) I think it's okay to print a warning, but probably shouldn't fail the build and disallow what is technical a valid input.

An error with the `--lang` flag returned the following message:
Detected lang specification starting with command flag `-`.
Is `--skip-git` meant to be an ember-cli command option?
This issue is likely caused by using the `--lang` flag without a specification.
Copy link
Member

@chancancode chancancode May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I think this shouldn't be in the normative part of the RFC anyway, but IMO this is working around a bug in the argument parsing, so exposing this much information about the limitation/bug/workaround in our argument parsing is more confusing to the user than it is helpful.

IMO, I think both of these cases should show something short like "The --lang flag requires a valid language code, see ember <command> help for more information.". In the second example, the --disable-analytics` bit is extra confusing, because it's not part of what the user typed and definitely a quirk of our internal argument parsing implementation.

Concretely, if the parsed value starts with a dash, I think it's just a bug in the parsing and we can show a message about the missing language code. Otherwise, we just validate against the ISO list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. I think a more consistent way to do this is when parse fails, print a message and either a descriptive help message with what the flag is for, or a link to a URL that has "common misunderstandings" if it is necessary. Trying to guess user intention is probably not going to scale very well.

@chancancode
Copy link
Member

chancancode commented May 29, 2020

What happens if no --lang flag is passed?

That would be the current/default behavior (no lang attribute in index.html), which is useful if you are going to deal with that some other way (via an internationalization plugin, etc).

Can a default lang setting be set in ~/.ember-cli or ~/.ember-cli.js?

Like any other Ember CLI options, it can be set in ~/.ember-cli, which the caveat that it will be passed to all Ember CLI commands, which fine as long as nothing else uses it for a conflicting meaning. It's an unfortunate limitation of the current Ember CLI implementation.

@chancancode
Copy link
Member

Thank you everyone for doing the investigation and putting together! It's very well written.

@Gaurav0
Copy link
Contributor

Gaurav0 commented May 29, 2020

That would be the current/default behavior (no lang attribute in index.html), which is useful if you are going to deal with that some other way (via an internationalization plugin, etc)

I don't think this is adequate. Ember needs to become accessible by default. Either a lang should be required or a default language selected.

@MelSumner
Copy link
Contributor

What happens if no --lang flag is passed?

Nothing will happen. It will be the same as it is now.

Can a default lang setting be set in ~/.ember-cli or ~/.ember-cli.js?

Why? It needs to go in the index.html file on the HTML tag.

@MelSumner
Copy link
Contributor

That would be the current/default behavior (no lang attribute in index.html), which is useful if you are going to deal with that some other way (via an internationalization plugin, etc)

I don't think this is adequate. Ember needs to become accessible by default. Either a lang should be required or a default language selected.

It’s backwards compatible & I wouldn’t suggest anything else for the MVP.

@chancancode
Copy link
Member

I don't think this is adequate. Ember needs to become accessible by default. Either a lang should be required or a default language selected.

I agree with the general sentiment but this RFC is a step in the right direction and doesn't need to fix everything at once. This allows some developers (who are aware of the issue, and only support a single language in the app) to start using the feature, allows us to the start teaching it in the tutorial, etc. And subject to the caveats I mentioned above, you can even set it as a default in your ~/.ember-cli.

In general, there a lot of other things like this that doesn't have a good, global default answer for everyone, but may have a good enough default answer for you. For example, in the case of ember new:

  • What LICENSE file to generate, or do you want one at all? (Set private field in package.json?)
  • Do you want us to generate a code of conduct? If so, which one to use? Or is this a private project, so it's not needed since it's already covered by internal company guidelines? Or do you want to delegate to the Ember community guidelines?
  • Technical things like what CI service do you want to use? Do you want to use TypeScript or not? What test framework?

For a lot of these questions, there aren't necessarily one good answer for a lot of these questions, and right now our infrastructure is not really setup to accommodate that kind of stuff. For the lang attribute in particular, setting a wrong default value could cause issues too, as mentioned in the RFC, and it would be quite heavy handed and presumptuous of us, IMO, to pick en or en-us as a default here.

We need a more general solution for these kind of things, probably something along the lines of an interactive ember new that allows you to state your preference for some of these things and remembers it. We also need a better way to store and pass along these values, as the caveats of the ~/.ember-cli "flash namespace" is really hindering us here.

A lot of these things are already in progress or on different people's radar. However, we don't have to block on those improvements to start making progress here. This is a good feature to add now, and when those other things get sorted out, they will just be implemented on top of what we started here.




## How we teach this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this RFC! I enjoy how well reasoned it is and its focus.

For that reason, I would like to see the "How we teach this" section expanded with some of the knowledge in the motivation section. The "Testing it out" was particularly illumating to me as a content creator in understanding the implications of the lang attribute.

We have a Language attribute section in the Ember Guides that could also be updated to mention the new flag?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@locks yes, that seems correct. I’ll work on it. Thanks for the feedback!

@abhilashlr
Copy link
Contributor

Amazing work on this RFC :)

Question: Would there be an error if the --lang flag value is en-us instead of en-US?

@chriskrycho
Copy link
Contributor

chriskrycho commented Jun 1, 2020

Can a default lang setting be set in ~/.ember-cli or ~/.ember-cli.js?

Why? It needs to go in the index.html file on the HTML tag.

The ~/.ember-cli and ~/.ember-cli.js files allow users to set default values of arguments for Ember CLI on their machines: if you have lang: 'fr' set, you never have to pass --lang='fr' explicitly. An example use case here: a team which always wants to default the language to fr could set their dev machine config up so that every new Ember dev gets it configured out of the box, and any new app they spin up always has it configured correctly.

@josephdsumner
Copy link
Contributor

josephdsumner commented Jun 1, 2020

@abhilashlr

Amazing work on this RFC :)

Question: Would there be an error if the --lang flag value is en-us instead of en-US?

Thanks! And no, there shouldn’t be an error for that example (or others related to case sensitivity). Here’s the relevant unit tests from the candidate:
image

Update to address feedback from @locks
@rajasegar
Copy link

Awesome job on the RFC, thanks a lot for putting this up.

@rwjblue
Copy link
Member

rwjblue commented Jun 8, 2020

After discussion at Friday's core team meeting, we decided to move this into final comment period.

🗣️💬⌛

@rwjblue rwjblue changed the title Ember New Lang ember new --lang Jun 8, 2020
@Gaurav0
Copy link
Contributor

Gaurav0 commented Jun 9, 2020

I don't think this is adequate. Ember needs to become accessible by default. Either a lang should be required or a default language selected.

I agree with the general sentiment but this RFC is a step in the right direction and doesn't need to fix everything at once. This allows some developers (who are aware of the issue, and only support a single language in the app) to start using the feature, allows us to the start teaching it in the tutorial, etc. And subject to the caveats I mentioned above, you can even set it as a default in your ~/.ember-cli.

i agree, it is a step forward and I don't want to hold this up. I withdraw my objection.

@luxzeitlos
Copy link

So should an app that uses ember-intl use this flag and define the "default" language or not?

@hergaiety
Copy link
Contributor Author

@luxferresum ember-intl sets the html tag's lang attribute anyway ember-intl/ember-intl#649, so if you plan to use that addon then you don't need to use this flag necessarily but it also won't conflict. So you can't go wrong here, is the idea.

As this relates to ember-intl

The popular localization library ember-intl does not conflict with the addition of this new Ember CLI flag. The addition of this flag offers some out of the box support where it was previously missing for new Ember apps. It is still recommended that globalized apps leverage ember-intl.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2020

We discussed this in todays core team meeting and are good to go here 🥰

@rwjblue rwjblue merged commit ed51170 into emberjs:master Jun 12, 2020
@sandstrom
Copy link
Contributor

sandstrom commented Jun 24, 2020

If the flag is used, would it make sense to output a console message on ember new telling people about ember-intl specifically, or to suggest a search in ember-addons (or similar website) to look for an addon with more i18n capabilities?

Right now it doesn't really do anything except setting a simple html attribute, so people may be disappointed that "it doesn't do more". That said, I don't think it needs to do more, internationalization is probably a better fit for an addon regardless. And ember-intl is an excellent addon!

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

Successfully merging this pull request may close these issues.

None yet