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

DEV: enable plain functions as helpers in Ember #22023

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

chancancode
Copy link
Contributor

@chancancode chancancode commented Jun 8, 2023

Rebased on top of #22095

Requested by @davidtaylorhq in https://github.com/discourse/discourse/pull/21899/files#r1221318682

This allows plain JavaScript functions to be used as template helpers. For now, it removes a bit of boilerplate from the helper files (see the truth-helpers refactor). Soon, with gjs/template imports (#21899), it will allow importing any JS functions into the template.

References

RFC: emberjs/rfcs#756
Update: emberjs/rfcs#788
Guides: ember-learn/guides-source#1924

@chancancode
Copy link
Contributor Author

Seems like the polyfill is having some trouble, looking 👀

@davidtaylorhq davidtaylorhq added the js-modernization Tasks related to our JS modernization project, including the Ember 4.0+ upgrade label Jun 8, 2023
@chancancode chancancode force-pushed the function-as-helpers branch 6 times, most recently from 4d8020a to 21341e0 Compare June 9, 2023 16:59
@chancancode
Copy link
Contributor Author

@davidtaylorhq got it working and updated the comment. Do you know if the failed system test is related, or is the test known to be flaky? It doesn't seem like there were any JavaScript errors, and looking at the screenshot in the artifacts, the expected text does seem to be rendered correctly. Maybe it's a timing issue?

@chancancode chancancode requested a review from davidtaylorhq June 9, 2023 17:17
chancancode added a commit to tildeio/discourse that referenced this pull request Jun 13, 2023
https://meta.discourse.org/t/updating-our-initializer-naming-patterns/241919

For historical reasons, Discourse has different initializers conventions
than standard Ember:

| Ember                 | Discourse          |                        |
| initializers          | pre-initializers   | runs once per app load |
| instance-initializers | (api-)initializers | runs once per app boot |

In addition, the arguments to the initialize function is different –
Ember initializers get either the `Application` or `ApplicationInstance`
as the only argument, but the "Discourse style" gets an extra container
argument preceding that.

This is confusing, but it also causes problems with Ember addons, which
expects the standard naming and argument conventions:

1. Typically, V1 addons will define their (app, instance) initializers
  in the `addon/(instance-)initializers/*`, which appears as
  `ember-some-addon-package-name/(instance-)initializers/*` in the
  require registry.

2. Just having those modules defined isn't supposed to do anything, so
  typically they also re-export them in `app/(instance-)initializers/*`,
  which gets merged into `discourse/(instance-)initializers/*` in the
  require registry.

3. The `ember-cli-load-initializers` package supplies a function called
  `loadInitializers`, which typically gets called in `app.js` to load
  the initializers according to the conventions above. Since we don't
  follow the same conventions, we can't use this function and instead
  have custom code in `app.js`, loosely based on official version but
  attempts to account for the different conventions.

The custom code that loads initializers is written with Discourse core
and plug-ins/themes in mind, but does not take into account the fact
that addons can also bring initializers, which causes the following
problems:

* It does not check for the `discourse/` module prefix, so initializers
  in the `addon/` folders (point 1 above) get picked up as well. This
  means the initializer code is probably registered twice (once from the
  `addon/` folder, once from the `app/` re-export). This either causes
  a dev mode assertion (if they have the same name) or causes the code
  to run twice (if they have different names somehow).

* In modern Ember blueprints, it is customary to omit the `"name"` of
  the initializer since `ember-cli-load-initializers` can infer it from
  the module name. Our custom code does not do this and causes a dev
  mode assertion instead.

* It runs what then addon intends to be application initializers as
  instance initializers due to the naming difference. There is at least
  one known case of this where the `ember-export-application-global`
  application initialize is currently incorrectly registered as an
  instance initializer. (It happens to not use the `/addon` folder
  convention and explicitly names the initializer, so it does not
  trigger the previous error scenarios.)

* It runs the initializers with the wrong arguments. If all the addon
  initializer does is lookup stuff from the container, it happens to
  work, otherwise... ???

* It does not check for the `/instance-initializers/` module path so any
  instance initializers introduced by addons are silently ignored.

These issues were discovered when trying to install an addon that brings
an application initializer in discourse#22023.

To resolve these issues, this commit:

* Migrates Discourse core to use the standard Ember conventions – both
  in the naming and the arguments of the initialize function

* Updates the custom code for loading initializers:
  * For Discourse core, it essentially does the same thing as
    `ember-cli-load-initializers`
  * For plugins and themes, it preserves the existing Discourse
    conventions and semantics (to be revisited at a later time)

This ensures that going forward, Ember addons will function correctly.
@chancancode chancancode force-pushed the function-as-helpers branch from 21341e0 to 33df532 Compare June 13, 2023 22:33
@github-actions github-actions bot added the chat PRs which include a change to Chat plugin label Jun 13, 2023
CvX pushed a commit that referenced this pull request Jun 15, 2023
https://meta.discourse.org/t/updating-our-initializer-naming-patterns/241919

For historical reasons, Discourse has different initializers conventions than standard Ember:

```
| Ember                 | Discourse          |                        |
| initializers          | pre-initializers   | runs once per app load |
| instance-initializers | (api-)initializers | runs once per app boot |
```

In addition, the arguments to the initialize function is different – Ember initializers get either the `Application` or `ApplicationInstance` as the only argument, but the "Discourse style" gets an extra container argument preceding that.

This is confusing, but it also causes problems with Ember addons, which expects the standard naming and argument conventions:

1. Typically, V1 addons will define their (app, instance) initializers in the `addon/(instance-)initializers/*`, which appears as `ember-some-addon-package-name/(instance-)initializers/*` in the require registry.

2. Just having those modules defined isn't supposed to do anything, so typically they also re-export them in `app/(instance-)initializers/*`, which gets merged into `discourse/(instance-)initializers/*` in the require registry.

3. The `ember-cli-load-initializers` package supplies a function called `loadInitializers`, which typically gets called in `app.js` to load the initializers according to the conventions above. Since we don't follow the same conventions, we can't use this function and instead have custom code in `app.js`, loosely based on official version but attempts to account for the different conventions.

The custom code that loads initializers is written with Discourse core and plug-ins/themes in mind, but does not take into account the fact that addons can also bring initializers, which causes the following problems:

* It does not check for the `discourse/` module prefix, so initializers in the `addon/` folders (point 1 above) get picked up as well. This means the initializer code is probably registered twice (once from the `addon/` folder, once from the `app/` re-export). This either causes a dev mode assertion (if they have the same name) or causes the code to run twice (if they have different names somehow).

* In modern Ember blueprints, it is customary to omit the `"name"` of the initializer since `ember-cli-load-initializers` can infer it from the module name. Our custom code does not do this and causes a dev mode assertion instead.

* It runs what then addon intends to be application initializers as instance initializers due to the naming difference. There is at least one known case of this where the `ember-export-application-global` application initialize is currently incorrectly registered as an instance initializer. (It happens to not use the `/addon` folder convention and explicitly names the initializer, so it does not trigger the previous error scenarios.)

* It runs the initializers with the wrong arguments. If all the addon initializer does is lookup stuff from the container, it happens to work, otherwise... ???

* It does not check for the `/instance-initializers/` module path so any instance initializers introduced by addons are silently ignored.

These issues were discovered when trying to install an addon that brings an application initializer in #22023.

To resolve these issues, this commit:

* Migrates Discourse core to use the standard Ember conventions – both in the naming and the arguments of the initialize function

* Updates the custom code for loading initializers:
  * For Discourse core, it essentially does the same thing as `ember-cli-load-initializers`
  * For plugins and themes, it preserves the existing Discourse conventions and semantics (to be revisited at a later time)

This ensures that going forward, Ember addons will function correctly.
This feature landed in Ember 4.5+, but this polyfill would allow
it to work on 3.25+

References

RFC: emberjs/rfcs#756
Update: emberjs/rfcs#788
Guides: ember-learn/guides-source#1924
Mainly to test that the polyfill is working, but it's a good
refactor anyway.
@chancancode chancancode force-pushed the function-as-helpers branch from 33df532 to 6235a7f Compare June 15, 2023 16:12
@github-actions github-actions bot removed the chat PRs which include a change to Chat plugin label Jun 15, 2023
Copy link
Contributor

@CvX CvX left a comment

Choose a reason for hiding this comment

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

🚀

@CvX CvX merged commit 0fa9252 into discourse:main Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
js-modernization Tasks related to our JS modernization project, including the Ember 4.0+ upgrade
Development

Successfully merging this pull request may close these issues.

3 participants