Skip to content

Add support to have ReactStyleFileName#32

Merged
alexgb merged 9 commits intoAltSchool:masterfrom
pswai:react-style-file
Jul 17, 2018
Merged

Add support to have ReactStyleFileName#32
alexgb merged 9 commits intoAltSchool:masterfrom
pswai:react-style-file

Conversation

@pswai
Copy link
Copy Markdown
Contributor

@pswai pswai commented Jul 11, 2018

The file name convention for Ember components is snake-case, however in React seems to be PascalCase. This PR adds support for ReactStyleFileName.

Problem

Let's we at the 2 major use cases of using React in Ember:

  • Gaining performance boost for certain part of UI
  • Migrating from Ember to React

For the first scenario, using Ember's convention makes sense because Ember and React are going to coexist. Having 2 file name conventions is confusing to developers.

However, for the second scenario, having 2 file name conventions becomes a good thing, because it shows clearly which components haven't been migrated yet. In fact, using Ember's convention for React components becomes awkward when importing components:

import React from 'npm:react';
import FancyCard from 'npm:third-party/core/FancyCard';
import TodoList from './todo-list'; // Inconsistent

Also, it is painful at the end of the project to have to rename all React components to PascalCase and update all the imports.

Proposed Solution

With support for ReactStyleFileName, users for the second scenario can write their React components this way:

import React from 'npm:react';
import FancyCard from 'npm:third-party/core/FancyCard';
import TodoList from './TodoList'; // Consistent

When the migration is done, no renaming is required since we have everything in place already. (Ok not yet, the npm: prefix can be removed with #27).

Caveat

Unfortunately we still can't use single-worded component in Handlebars:

{{!-- Doesn't work --}}
{{avatar}}

However we can use it with the react-component component:

{{!-- This works --}}
{{react-component 'avatar'}}

Comment thread README.md Outdated
**This is ALPHA Software**

This was built as a prototype to evaluate using react inside of our Ember apps. We are not yet using it in production. PRs and constructive questions and comments via [GitHub issues](https://github.com/AltSchool/ember-cli-react/issues/new) are highly encouraged.
This was built as a prototype to evaluate using react inside of our Ember apps.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Most of the changes were caused by Prettier (apparently I missed this file last time)

Comment thread README.md
highly recommended to have clean React component tree whenever possible for best
performance.

## Using File Name Convention for React
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This section is the real content addition

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like a good explanation of why, but doesn't really make it clear what ember-cli-react is providing in terms of file name support. I think it should make it clear that 1) you can name your jsx files in PascalCase and 2) either import them in templates using standard dasherized syntax or refer to them there using PascalCase with the aid of react-component.

Copy link
Copy Markdown
Contributor

@alexgb alexgb left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed PR explanation. Your reasoning makes sense to me. I've left a few comments.

});

it('supports React-style component file name', function() {
this.render(hbs`{{react-component "react-style-file-name"}}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should have a test for `{{react-component "ReactStyleFileName"}} as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh right, that's a valid test

Comment thread addon/resolver.js Outdated
// A React-style file name is capitalized camel-cased.
resolveReactStyleFile(parsedName) {
const originalName = parsedName.fullNameWithoutType;
parsedName.fullNameWithoutType = Ember.String.classify(originalName);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of mutating parsedName can we copy it?

const parsedNameWithReactStyle = Object.assign(
  {}, 
  parsedName, 
  { fullNameWithoutType: Ember.String.classify(originalName) }
);

This should mean you can get rid of line 51.

Comment thread README.md
highly recommended to have clean React component tree whenever possible for best
performance.

## Using File Name Convention for React
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like a good explanation of why, but doesn't really make it clear what ember-cli-react is providing in terms of file name support. I think it should make it clear that 1) you can name your jsx files in PascalCase and 2) either import them in templates using standard dasherized syntax or refer to them there using PascalCase with the aid of react-component.

@pswai pswai force-pushed the react-style-file branch from e68c654 to 13e3ac4 Compare July 16, 2018 07:21
@pswai
Copy link
Copy Markdown
Contributor Author

pswai commented Jul 16, 2018

@alexgb I have added a lot of tests to cover several test cases, mainly around the cases when conflict occurs.

The basis of all conflict resolution is "prefer React component over Ember component".

Scenarios:

  1. Two React components: SameName.jsx vs same-name.jsx
    SameName.jsx is selected

  2. One React, one Ember, different casing: SameName.jsx vs same-name.js
    SameName.jsx is selected

  3. One React, one Ember, same casing: same-name.jsx vs same-name.js
    Broken. same-name.js is overwritten with the result of same-name.jsx transpilation. The end result is same-name.jsx being selected

Ember component can never have PascalCase file name so only 3 scenarios exist.

This behaviour was chosen to provide a simple resolution strategy for the following use cases:

{{!-- Must be React component --}}
{{react-component "user-avatar"}}

{{!-- Must be React component --}}
{{react-component "UserAvatar"}}

{{!-- Can be either Ember or React component --}}
{{user-avatar}}

{{!-- NOT OK! --}}
{{UserAvatar}}

We can try to be smart and determine the right file by the different use case above, but I think it is overly complicated and not unnecessary.

@pswai pswai force-pushed the react-style-file branch from ca551f5 to 8911fca Compare July 17, 2018 05:34
Copy link
Copy Markdown
Contributor

@alexgb alexgb left a comment

Choose a reason for hiding this comment

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

Looks good, left one small comment.

I also worked on simplifying the Readme a bit, including some edits to the text here about file naming. I'll open a separate PR for these changes, perhaps as part of bringing to 1.0. cfc52a5

Comment thread addon/resolver.js Outdated

// This resolver method attempt to find a file with React-style file name.
// A React-style file name is in PascalCase.
resolveReactStyleFile(parsedName) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the resolve* methods on the resolver determined at runtime based on factory types. So naming a method this way also adds a new factory type of react-style-file:*. Since I don't think that's the intention here I would suggest marking as a private method _resolveReactStyleFile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah great catch. I'll update this part :)

@alexgb alexgb merged commit 6b58a8f into AltSchool:master Jul 17, 2018
@alexgb
Copy link
Copy Markdown
Contributor

alexgb commented Jul 17, 2018

@pswai thanks for the great PR writeup on this one

@pswai pswai deleted the react-style-file branch July 17, 2018 06:25
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

Successfully merging this pull request may close these issues.

2 participants