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

feat: modernize addon, add optional Ember mixin props to decorator, testing helper #62

Open
wants to merge 69 commits into
base: master
Choose a base branch
from

Conversation

shamrt
Copy link

@shamrt shamrt commented Jun 10, 2021

REVIEWER NOTE: Not nearly as big as it seems. The yarn.lock file consists of 8,041 additions and 5,810 deletions.

Description

This PR does two major things:

Other changes

  • Fixes and refactors the horribly busted tests.
    • Added a test for React hooks.
  • Replaces testdouble with sinon due to some issues related to ember-auto-import that I couldn't resolve. (I'm aware of your involvement with the testdouble community, so tried to avoid this, but wasn't able to.)
  • Removes ember-metric due to some odd recursion errors. (Again, I fought this for a while, but was unsuccessful in finding a way around it.)
  • Tidied up various configs and updated some JS/Ember syntax.

Background

I had originally planned to migrate the addon to use Glimmer components (resolving #41), but it turns out that is infeasible due to (1) Glimmer's drastically simplified API and (2) a combination of limitations within the React DOM API and the nature of the DOM itself. The next best thing was to provide the means for modifying aspects of the wrapper node.

shamrt added 30 commits June 1, 2021 14:36
BREAKING CHANGE: Drop Node v10 support
better compatibility with ember-auto-import
several dependencies complained about it missing
```

With that set up, you can inject references to a service the same way you can with an Ember component
You can inject references to a service the same way you can with an Ember component
Copy link
Author

Choose a reason for hiding this comment

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

I believe these caveats are outdated, given the addon would no longer support Ember v3.1, so I removed them.

@shamrt shamrt marked this pull request as draft June 10, 2021 17:39
@shamrt shamrt marked this pull request as ready for review June 11, 2021 13:26
@shamrt
Copy link
Author

shamrt commented Jun 11, 2021

I found some small issues in live testing, and fixed them, so I think this is now ready for review

@shamrt
Copy link
Author

shamrt commented Jun 14, 2021

In live testing, we found another issue related to the Ember internals MutableCell class, which is now resolved

@zavan
Copy link

zavan commented Jun 15, 2021

Thank you for this!
I'll do some experimentation and let you know if I find any issues.

@shamrt shamrt changed the title feat: modernize addon, add optional Ember mixin props to decorator feat: modernize addon, add optional Ember mixin props to decorator, testing helper Apr 6, 2022
@LitterStarXin
Copy link

Hi @shamrt. I am testing react-ember-component locally and have some questions need your help.

  1. I tested data/action down works as expect. But if I want trigger action up, like call ember function from react child component, is that be supported as well?
  2. May I know is this library can never support Glimmer for some reasons?

Hope to hearing from you and thanks ahead.

@tehhowch
Copy link

@shamrt For whatever reason, production builds (i.e. those where Terser did things) don't work quite right when mapping over an array

I'm on Ember 3.28 with ember-auto-import 2.4.2 and ember-cli-terser 4.0.2.

The ember template that invokes an annotated React component (other ember stuff omitted):

{{#if this.someArrayFromEmber}}
  <MyReactComponent @json={{this.someArrayFromEmber}} />
{{/if}}

The annotated React component:

import React from 'react';
import WithEmberSupport from '@rewardops-forks/ember-react-components';
// eslint-disable-next-line no-unused-vars
import PreviewDetails from '../react/preview-details';

@WithEmberSupport
export default class MyReactComponent extends React.Component {
  render() {
    const { json } = this.props;

    return (
      <PreviewDetails entities={json} />
    );
  }
}

And <PreviewDetails /> is a normal react component:

import React from 'react';
import PropTypes from 'prop-types';

const PreviewDetails = ({ entities }) => {
  function pickComponent(e) {
    if (!e) return <p>No active entity</p>;

    if (e.entityType === 'VIEW') {
      return <OtherReactFunctionalComponent
        key={e.id}
        view={e}
      />;
    }
    return `Not Yet Supported: "${e.entityType}"`;
  }

  console.error(Array.isArray(entities)); // true in dev build, false in prod build!

  return (
    <div className={`preview-details-container`}>
      { entities.map((e) => pickComponent(e)) } // dies here in prod build, since "entities" transpiles into an object with a "value" property that is the "entities" when running a dev build
    </div>
  );
};

PreviewDetails.propTypes = {
  entities: PropTypes.array,
};

PreviewDetails.defaultProps = {
  entities: [],
};

export default PreviewDetails;

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.

4 participants