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

WIP: Rich Text Emails #33779

Closed

Conversation

proteusvacuum
Copy link
Contributor

Note: Not for merging!

This PR is to show what I'm doing to try to get ckeditor to be included after installation with yarn.

When loading this in the browser, I get:

Uncaught TypeError: Unable to process binding "with: function(){return standalone_content_form }"
Message: Unable to process binding "with: function(){return html_message }"
Message: Unable to process binding "ckeditor: function(){return nonTranslatedMessage }"
Message: can't access property "create", CKEditor is undefined

Steps to reproduce:

  • Check out this branch, install dependencies and run migrations
  • Add your domain to the RICH_TEXT_EMAILS feature flag
  • Create a new broadcast (Messaging -> Broadcasts -> New)
  • Select "Email" as the broadcast type. The ckeditor will then try to load, but will fail (and will present the error message above in the console)
  • If you check out the second-to-last commit 2872701 and follow the same steps, you will see the editor. This works as I was previously adding the javascript directly in the repo.

Other things I've tried:

  1. Adding a shim in hqwebapp/js/bootstrap5/requirejs_config.js
       "ckeditor/build/ckeditor5-dll": {
            exports: 'ClassicEditor',
        }
  1. Tried all the different permutations listed here: A bit confusing syntax required for RequireJS ckeditor/ckeditor5#545 (comment) but these seem to all reference an older way this is being packaged.

@dimagimon dimagimon added dependencies Pull requests that update a dependency file reindex/migration Reindex or migration will be required during or before deploy labels Nov 21, 2023
'jquery',
'underscore',
'knockout',
'ckeditor5/build/ckeditor5-dll',
Copy link
Contributor

@millerdev millerdev Nov 21, 2023

Choose a reason for hiding this comment

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

I think the problem is that requirejs (hqDefine) does not know where to find this library. Does this need to be added to thirdPartyGlobals as outlined in the RequireJS Migration Guide?

Maybe here and here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @millerdev - thank you this was very helpful!

The issue was that I hadn't defined the correct shims in the requirejs_config files.

You can see in the commit I just pushed: 17fd853 how this works now.

However, in the CKEditor5 build from npm, they define every single feature as a (very granular) plugin: https://ckeditor.com/docs/ckeditor5/latest/installation/advanced/alternative-setups/dll-builds.html#using-a-dll-build

To allow for every feature that is required to make this useful for the thing I'm building, I think I'll have to add about 50 new shims to this list. If you and @orangejenny still think this is preferable to directly including the build that I was able to use in the codebase, I'm happy to go this route. However, it does seem to be quite a bit more work, and adds a lot of boilerplate in the codebase that doesn't necessarily need to be there.

I'd love to hear your thoughts here!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oof, 50 is a lot, and I'm guessing that identifying the exact features might be a task in and of itself. But I do lean towards adding the many, many shims. My main concern is that adding ckeditor directly to the codebase means we can't take advantage of tools to notify us of new versions and, more importantly, any CVEs identified.

I defer to Daniel if he disagrees, as he's more involved in dependency management than I am.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @orangejenny ! Sounds good. I have a good list of the features (as outlined in the spec doc), and I'll proceed with adding these through npm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jenny on the reasons to use shims, making that preferable to direct include.

@dimagimon dimagimon added the Risk: High Change affects files that have been flagged as high risk. label Nov 22, 2023
@proteusvacuum proteusvacuum deleted the fr/rich-text-emails-yarn branch November 27, 2023 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file reindex/migration Reindex or migration will be required during or before deploy Risk: High Change affects files that have been flagged as high risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants