Skip to content
This repository was archived by the owner on Jun 8, 2019. It is now read-only.

Conversation

@mattkime
Copy link
Contributor

@mattkime mattkime commented Dec 22, 2016

addresses #80

adds ability to configure additional metadata fields and whether they're required.

fields: For specifying additional metadata fields and whether they're required. Takes an object with field names as key names, which specify whether they're required. fields: { metadata: { required: true }, otherdata: {required: false} }

const expectedMessages = fs.readFileSync(path.join(fixtureDir, 'expected.json'));
const actualMessages = fs.readFileSync(path.join(fixtureDir, 'actual.json'));
assert.equal(trim(actualMessages), trim(expectedMessages));
const expectedMessages = require(path.join(fixtureDir, 'expected.json'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this test to rely on deepEqual instead of text string content. different object key order was breaking tests

src/index.js Outdated
let key = getMessageDescriptorKey(keyPath);

if (DESCRIPTOR_PROPS.has(key)) {
if (Object.keys(fields).includes(key)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is breaking tests in older node versions

@ericf
Copy link
Collaborator

ericf commented Dec 22, 2016

@mattkime thanks for working through this! I want to have a deeper understanding of your use cases so I can internalize them. Could you provide some real world examples of defineMessages and <FormattedMessage> that shows how you plan to use this feature?

@mattkime
Copy link
Contributor Author

mattkime commented Dec 22, 2016

We'll have a field that references a story or epic in our ticketing system - jira='MW-123'. This helps provide context for translating to new languages down the road.

@ericf
Copy link
Collaborator

ericf commented Dec 22, 2016

I'm most interested to see a JSX and/or JS example of exactly what you're thinking.

@mattkime
Copy link
Contributor Author

mattkime commented Dec 22, 2016

@ericf Does this clarify?

<FormattedMessage
     id='abc1234'
     description='card title'
     jira='MW-1234'>
     Article: {title}
</FormattedMessage>

but this is obviously only one side. The other half is extracting the data and providing it to the translators.

@ericf
Copy link
Collaborator

ericf commented Dec 22, 2016

Hmm… there's something here I'm not liking about expanding the props of <FormattedMessage>. But I do see the value in structured metadata. What if description could be any value as long as it can be statically evaluated and is JSON-serializable string or object?

<FormattedMessage
  id='abc1234'
  description={{
    text: 'card title',
    jira: 'MW-1234',
  }}
  defaultMessage='Article: {title}'
/>

And this would extracted to:

[{
  "id": "abc1234",
  "description": {
    "text": "card title",
    "jira": "MW-1234"
  },
  "defaultMessage": "Article: {title}"
}]

@mattkime
Copy link
Contributor Author

mattkime commented Dec 22, 2016 via email

@mattkime
Copy link
Contributor Author

I verified that this works for extraction as long as I get rid of a trim() call.

The next step is providing for some sort of verification of the description fields. i'd like to provide a function to enforceDescriptions.

@ericf
Copy link
Collaborator

ericf commented Dec 23, 2016

I verified that this works for extraction as long as I get rid of a trim() call.

The description should be type-checked and only call trim() if it's a string.

The next step is providing for some sort of verification of the description fields. i'd like to provide a function to enforceDescriptions.

This would likely only be possible if you use Babel via its API. Since .babelrc files need to be JSON, you can't provide a function. I think verifying the existence of the description is within the scope of this plugin, but validating its contents should be up to your build to post-process the .json files.

@ericf
Copy link
Collaborator

ericf commented Dec 29, 2016

Closing in favor of #87

@ericf ericf closed this Dec 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants