Skip to content

[DRAFT] webpack update - #1512

Merged
muselesscreator merged 19 commits into
openedx:masterfrom
muselesscreator:webpack
Oct 22, 2020
Merged

[DRAFT] webpack update#1512
muselesscreator merged 19 commits into
openedx:masterfrom
muselesscreator:webpack

Conversation

@muselesscreator

@muselesscreator muselesscreator commented Oct 1, 2020

Copy link
Copy Markdown
Contributor

TL;DR -
Update our build process for ORA2 JS to use webpack, thus allowing use of ES6 tools/syntax.
Uses frontend-build in order to closely align with MFE build practices.

What changed?
o.0

no more requireJS imports.
All JS is imported/exported from files.
npm configuration files for webpack builds based on frontend-build.
webpack build commands in Makefile.

ESlint styling changes, where feasible/easy/automatic.
Note: There are a number of remaining issues that are currently ignored by disabling rules in the eslint config that ABSOLUTELY should be addressed in the future (especially number of classes exported from a given file).

Developer Checklist

JIRA: JIRA-5190

FIY: @edx/masters-devs-gta

@jansenk jansenk left a comment

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've read through the changes and I have no concerns

Comment thread karma.conf.js
// start these browsers
// available browser launchers: https://npmjs.org/browse/keyword/karma-launcher
browsers: ['PhantomJS'],
browsers: ['HeadlessChrome'],

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.

Why this change?

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.

clearing warnings. PhantomJS not supported any more if I recall

@muselesscreator
muselesscreator force-pushed the webpack branch 2 times, most recently from c9a1309 to 59f5766 Compare October 19, 2020 20:54

@nsprenkle nsprenkle left a comment

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.

Overall looks good, pending continued successful smoke test and removal of console.logs. Random questions and comments (largely for my own curiosity) peppered throughout.

Comment thread Makefile
Comment thread env.yml
Comment thread webpack.dev.config.js
Comment thread openassessment/xblock/studio_mixin.py
Comment thread openassessment/xblock/static/js/src/lms/oa_datefactory.js
Comment thread openassessment/xblock/static/js/src/studio/oa_edit_rubric.js
Comment thread openassessment/locale/en/LC_MESSAGES/django.po Outdated
Comment thread openassessment/xblock/static/js/spec/studio/oa_edit_settings.js Outdated
Comment thread openassessment/xblock/static/js/src/lms/oa_response.js
Comment thread openassessment/xblock/static/js/src/studio/oa_edit_assessment.js Outdated
@muselesscreator
muselesscreator merged commit 4c77d87 into openedx:master Oct 22, 2020
jansenk added a commit that referenced this pull request Mar 7, 2022
fix: restore original tinymce behavior, fix trailing slash

In PR #1512 we introduced the bug by improperly combining the "dataHandler" methods into one method and only setting/checking src rather than also including href
In PR #1794 we attempted to fix the issue by fixing the order of the to->from in the dataHandler. We also did a more specific replacement of <a href=<url> and <img src=<url> This fixed the issue of inserting bad links and images, but the static url const used in the "load all content" transform was missing a slash resulting in incorrect "resolved" URLs. Thankfully according to the read replica, there are only 3 of those that currently exist.

Removing the handlers for EditImage and EditLink. Originally their purpose was so that you would only ever see the "static" link. When the image/link was saved, we'd convert it from the static link to the real link, and then when you would click "edit" we'd transition back from the real link to the "static" link. I'm getting rid of that. The URL presented in the edit dialog should actually be the URL and having it go back and forth hides the actual real value of the URL. Unnecessary and potentially confusing.

Removing the specific <a href=<url> and <img src=<url> filtering for the full content transform. It's nice, and it will potentially prevent issues where we have a /static/ that we don't want transformed, but unfortunately having the global replace is the current only way we can support resolving static URLs in audio or video embeds. (see https://openedx.atlassian.net/browse/AU-544) Rather than adding those specific "special cases" it seems like it's probably just easier to do the more general resolve?
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.

3 participants