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

issue/2824-jsx Changed bundler and added React support #2829

Closed
wants to merge 34 commits into from

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Jul 5, 2020

#2824
Will provide an almost seamless migration away from build-side requirejs and AMD modules to ES6 modules and add react support to the framework.

Facilitates

  • future proof
    • import and export directives
    • beyond ES8 code
    • react templates
  • development improvements
    • much faster compilation and recompilation times on all grunt tasks, this is as a cache is now generated for the build process
    • supports larger code bases
    • move towards supporting intellisense in code editors
  • backward-compatibility
    • ES6 modules are reregistered in the browser as AMD modules, so var Adapt = require('core/js/adapt'); etc will still work in the console and for other plugins
    • we can now rework Adapt to ES6 modules whilst retaining support for the existing AMD modules
    • still ie11 compatible

Downsides

  • External modules such as libraries/mediaelement-fullscreen-hook.js, which are required by internal modules and require internal modules are now considered circular dependencies and will break the loading process. A temporary fix is included until that module can be moved back into the js folder
  • Jumping to class function line numbers from the code inspector in the debugger is out of order as the define/require directive conversions moves the line numbers
  • Currently this will only work with transpilation back to ES5 as calling an ES6 constructor function without the new keyword is not allowed - which is an essential part of the way Backbone constructs classes.

Added

These plugins achieve react templating and integration in the same way we have been doing with handlebars:

Removed

  • requirejs as bundler in grunt
  • babel grunt task is now integrated into the rollup process
  • requirejs config.js file has been moved to the javascript grunt task config

Fixed

  • QuestionView has two different return statements, so cannot resolve to one export default directive

Testing

  • Use master of all plugins and accordion branch issue/2824-jsx by running:
git clone https://github.com/adaptlearning/adapt_framework 2824
cd 2824
git checkout issue/2824-jsx
adapt devinstall && npm install
cd src/components/adapt-contrib-accordion
git checkout issue/2824-jsx
cd ../../../
grunt dev

Example

Shows fast compilation times, import/export, debugging and require functioning with an ES6 module in the client-side:

rollup-es-modules2

Templates

Supports react 'templates' as near as I could get it to our current handlebars templates - separate, override-able etc. This change does not require a wholesale switch to React views but instead uses our current Backbone views and React as a templating engine. This means that some of the usual functionality of React views, such as the state object, aren't used to cause view re-render, instead the this.changed() function should be called to re-render a Backbone View with a React template.

Here are a few reasons for using React as a templating engine only:

  1. Our entire architecture uses Backbone events heavily - which React views don't support.
  2. Many of our bugs arise from jQuery DOM manipulation and using React to manage the DOM manipulation removes those bugs whilst introducing much more flexibility in the view <> state updates, this is as each manipulation is now automated and no longer needs to be coded by hand.
  3. Introducting jsx as a templating engine allows the upskilling requirement to be confined to templates only, which should be easier and less daunting for developers to confront.
  4. We will be able to convert to React at our leisure by not mandating a wholesale view layer rewrite.
  5. As React will be a part of the architecture we can better experiment with dropping Backbone as a large part of the architectural work is already complete.

Usage

A React version of the accordion lives here pr / branch

// This is an example template file in **/templates/**/name.jsx
// It will get registered in Adapt by filename as handlebars templates currently do
// Templates will override from theme, menu, extension, component as usual
// There is no partials distinction with the react templates, they are all just templates which can be called from one another

export default function (model, view) {
  return <div></div> // React component
};
// This is a template file in components/adapt-contrib-accordion/templates/accordion.jsx

// These functions help handlebars integration
import Adapt from 'core/js/adapt';
import { 
  compile, // Compile handlebars into html: compile(template, data);
  classes, // Helper for joining and filtering classes
  templates, // Lookup other react templates: templates[name](model, view);
  html, // Render html as react html:  html(`<div>new div</div>`); or html(compile('{{#if _prop}}<div>new div</div>{{/if}'));
  partial // Use a handlebars partial: partial(name, data);
} from 'core/js/reactHelpers';

export default function (model, view) {
  const data = model.toJSON();
  data._globals = Adapt.course.get('_globals');
  return <div className="component__inner accordion__inner">
    {templates.component(model, view)}
    <div className="component__widget accordion__widget">
      {data._items.map(({ _graphic, _classes, title, body, _index, _isVisited, _isActive }, index) => {
        return <div key={_index} className={classes([
              "accordion__item",
              _graphic.src && 'has-image',
              _classes && _classes
          ])} data-index={_index}>
          <button onClick={view.onClick.bind(view)} id={`${data._id}-${index}-accordion-button`} className={classes([
            'accordion__item-btn',
            'js-toggle-item',
            _isVisited && 'is-visited',
            _isActive ? 'is-open is-selected' : 'is-closed'
          ])} aria-expanded={_isActive ? 'true' : 'false'}>
            <div className="accordion__item-btn-inner">
              <div className="accordion__item-icon">
                <div className="icon"></div>
              </div>
              <div className="accordion__item-title">
                <div className="accordion__item-title-inner">
                  {html(title)}
                </div>
              </div>
            </div>
          </button>
          <div className="accordion__item-content" role="region" aria-labelledby={`${data._id}-${index}-accordion-button`}>
            <div className="accordion__item-content-inner">
              {body &&
              <div className="accordion__item-body">
                <div className="accordion__item-body-inner">
                  {html(compile(body))}
                </div>
              </div>
              }
              {_graphic.src &&
              <div className={classes([
                  'accordion__item-image-container',
                  _graphic.attribution && 'has-attribution'
              ])}>
                <img className="accordion__item-image" src={_graphic.src} aria-label={_graphic.alt} aria-hidden={_graphic.alt ? 'true' : 'false'} />
                {_graphic.attribution &&
                <div className="component__attribution accordion__attribution">
                  <div className="component__attribution-inner accordion__attribution-inner">
                    {html(_graphic.attribution)}
                  </div>
                </div>
                }
              </div>
              }
            </div>
          </div>
        </div>
      })}
    </div>
  </div>
}
// Example from accordion.
// adaptView is extended to support react templates, so all Adapt views can use react template as below.

import ComponentView from 'core/js/views/componentView';

class AccordionView extends ComponentView {

  preRender() {
    this.listenTo(this.model.getChildren(), {
      'all': this.changed // call changed to rerender
    });
  }

  /** missing code */

}

AccordionView.template = 'accordion.jsx'; // Just update the template name to use the react template

export default AccordionView;

You can see the core templates are using react versions https://github.com/adaptlearning/adapt_framework/tree/issue/2824-jsx/src/core/templates which means you can update the models for page, article and block on the fly and the views will update on the page:

var Adapt = require('core/js/adapt'); 
Adapt.findById('co-05').set('displayTitle', 'new title');
Adapt.findById('a-05').set('displayTitle', 'testing');
Adapt.findById('b-35').set('body', 'testing');

Release note

Remove page.jsx, article.jsx and block.jsx before merging as these are likely to have larger impacts on course content that should be avoided in the short-term. We should aim for component level react views to start.

@oliverfoster oliverfoster changed the title issue/2824 Replaced requirejs with rollup for module bundler and added React support issue/2824-jsx Replaced requirejs with rollup for module bundler and added React support Jul 6, 2020
@oliverfoster oliverfoster self-assigned this Jul 6, 2020
@oliverfoster oliverfoster changed the base branch from master to issue/2824 July 8, 2020 06:34
@oliverfoster oliverfoster changed the title issue/2824-jsx Replaced requirejs with rollup for module bundler and added React support issue/2824-jsx Added React support to issue/2824 Jul 8, 2020
@brian-learningpool
Copy link
Member

Great work!

* @param {string} html
*/
export function html(html, ref = null) {
let node = html ? HTMLReactParser(html) : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, please fix when undefined is passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

done in 9b3a7b0

* @param {...any} args List of classes
*/
export function classes(...args) {
return args.filter(Boolean).join(' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this accept an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

done in 9b3a7b0

Copy link
Member Author

Choose a reason for hiding this comment

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

updated core classes to match in 327a793

@oliverfoster oliverfoster changed the base branch from issue/2824 to master August 3, 2020 10:23
@oliverfoster oliverfoster changed the title issue/2824-jsx Added React support to issue/2824 issue/2824-jsx Changed bundler and added React support Aug 3, 2020
Copy link

@lc-alexanderbenesch lc-alexanderbenesch left a comment

Choose a reason for hiding this comment

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

code 👍
testing 👍

@oliverfoster
Copy link
Member Author

@tomgreenfield Just waiting on you before I remove the page, article and block react templates.

src/core/js/adapt.js Show resolved Hide resolved
@chucklorenz
Copy link
Member

@olivermartinfoster per our 16/09 chat,
Console error after running grunt build and grunt server:
HTMLReactParser is not defined

@oliverfoster oliverfoster added the on hold do not merge label Sep 23, 2020
@oliverfoster
Copy link
Member Author

Will rebase this against #2923 once that pr is accepted

@moloko moloko added this to the Adapt v6 milestone Oct 21, 2020
@oliverfoster
Copy link
Member Author

Replaced with #2963

@oliverfoster oliverfoster deleted the issue/2824-jsx branch November 2, 2020 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants