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

Export editor as web component #3554

Merged
merged 38 commits into from
Apr 22, 2024
Merged

Conversation

CodingDive
Copy link
Contributor

@CodingDive CodingDive commented Mar 25, 2024

In follow up PRS, we'll need to define a few more attributes for the web component.

For now, in its first iteration, the PR should be ready for review.

Mikey Stengel added 4 commits March 22, 2024 13:46
* Get rid of next useRouter dependency
* Load katex styles dynamically (remove global JSX)
* Don't point setAppElement to #__next, and define a "serlo-root" id ourselves. In the frontend, this is the HTML root, while for editor package consumers it will be the editor itself.
@CodingDive CodingDive requested review from elbotho and hejtful March 25, 2024 14:32
Copy link

vercel bot commented Mar 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
frontend ✅ Ready (Inspect) Visit Preview Apr 22, 2024 8:40am

Copy link
Contributor

github-actions bot commented Mar 25, 2024

📦 Next.js Bundle Analysis for @serlo/frontend

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 104.75 KB (🟡 +390 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Twenty-eight Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/[...slug] 83.69 KB (🟡 +1 B) 188.43 KB
/___bot_or_not 95.75 KB (🟡 +1 B) 200.5 KB
/___editor_preview 489.76 KB (🟢 -214 B) 594.51 KB
/___exercise_folder_dashboard/[id] 48.69 KB (🟡 +56 B) 153.44 KB
/___old_comments 101.4 KB (-1 B) 206.15 KB
/biologie 158.9 KB (🟡 +1 B) 263.65 KB
/chemie 158.89 KB (🟡 +1 B) 263.64 KB
/content-only/[...slug] 75.05 KB (🟡 +1 B) 179.8 KB
/discussions 95.85 KB (-1 B) 200.6 KB
/editor 105.43 KB (-3 B) 210.18 KB
/entity/create/[type]/[taxonomyId] 490.78 KB (🟢 -560 B) 595.53 KB
/entity/repository/add-revision/[...id] 490.03 KB (🟢 -533 B) 594.78 KB
/entity/repository/compare/[entity_id]/[revision_id] 88.13 KB (🟡 +1 B) 192.88 KB
/event/history 130.86 KB (-1 B) 235.61 KB
/event/history/[...slug] 131.86 KB (-1 B) 236.61 KB
/event/history/user/profile/[username] 134.29 KB (-1 B) 239.04 KB
/gleichungs-app 164.54 KB (-3 B) 269.29 KB
/informatik 158.9 KB (🟡 +1 B) 263.65 KB
/lerntipps 158.89 KB (🟡 +1 B) 263.64 KB
/license/detail/[id] 58.71 KB (🟡 +1 B) 163.46 KB
/mathe 158.89 KB (🟡 +1 B) 263.64 KB
/nachhaltigkeit 158.9 KB (🟡 +1 B) 263.65 KB
/page/create 490.04 KB (🟢 -536 B) 594.79 KB
/taxonomy/term/create/[parent_id]/[id] 489.78 KB (🟢 -489 B) 594.53 KB
/taxonomy/term/update/[id] 489.72 KB (🟢 -470 B) 594.47 KB
/user/notifications 133.15 KB (-1 B) 237.9 KB
/user/profile/[username] 177.26 KB (-1 B) 282.01 KB
/user/settings 488.73 KB (🟢 -485 B) 593.48 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@CodingDive CodingDive marked this pull request as draft March 25, 2024 14:45
Mikey Stengel added 2 commits March 25, 2024 18:18
…r exercise-group

* Remove auth, userTools and license from exercise-group static renderer in editor package
* Define custom vite config
* Add boilerplate (tsconfig/eslint/prettier)
* Ensure build works
* Prepare github workflow for publishing
* Revert changes from vite build config in editor package
Mikey Stengel added 2 commits April 2, 2024 12:15
* Output types of editor into /dist folder directly
* rollup all the types into one bundle
* Remove unneeded type assertions (type errors were caused by import problems)
Copy link

@yorrd yorrd left a comment

Choose a reason for hiding this comment

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

focused on the webcomponent part

"main": "./dist/serlo-editor-web-component.js",
"module": "./dist/serlo-editor-web-component.js",
"types": "./dist/editor-web-component.d.ts",
"files": [
Copy link

Choose a reason for hiding this comment

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

this leads to a lot of things being exposed through the package under the dist directory. Common practice would be exposing those types via tree shakable semantic subpaths and strip internals.

image

Understandable if a semantic structure won't be part of the 1.0 release since tree shaking would really not bring any benefit at the moment and would mostly just bloat the build process atm. I'm bringing this up because it's usually good practice to have well structured import paths - my suggestion for now would be just not exposing the dist directory in the final bundle but rather only the index export (ln 15). Not a hugely important topic.

// Could probably remove the export entirely, as the customElement is registered
// below.
export class EditorWebComponent extends HTMLElement {
private reactRoot: ReactDOM.Root | null = null
Copy link

Choose a reason for hiding this comment

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

for a lack of a better place to put this, I'll have it at the react dom root:

In vanilla JS applications, there is no process.env.. However, the bundle assumes that it exists and bears "production" or "staging". Easiest workaround is to include process = {env: 'production'} in the banner of the bundle if the environment does not already worry about the global process variable through another bundler. The "real" solution would be not to rely on that variable in the bundle's output but I'm not sure as to how realistic that is on the short term.

From experience, that's usually a big pain point for package distributors since bundling issues hold back adoption. Putting a workaround in one's own code looks super ugly and is frowned upon, still holding back adoption. On the other hand, even excalidraw has this issue since they also bundle react in their component, but notably they don't pretend to be agnostic and just deliver a react component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you recommend me to put this global variable into the vite.config or how should we proceed here?

Copy link

Choose a reason for hiding this comment

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

sorry missed github notifications... not too familiar with vite's build system but if you can add global constants that way, that would probably be good. Anywhere that's not in the actual code because it's only for "glueing stuff together"

Copy link

Choose a reason for hiding this comment

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

sorry completely missed github notifications. I'm not too familiar with vite's build system. That being said, anywhere that's at compile time to inject the thing into the final bundle. Build config sounds fair enough to me without knowing details about vite stuff.

packages/editor-web-component/src/editor-web-component.tsx Outdated Show resolved Hide resolved
packages/editor-web-component/src/editor-web-component.tsx Outdated Show resolved Hide resolved
packages/editor-web-component/src/editor-web-component.tsx Outdated Show resolved Hide resolved
@yorrd
Copy link

yorrd commented Apr 6, 2024

I just completely forgot to add one thing @CodingDive : we need a way to expose the current editor state. Good practice is to dispatch an event on any editor change. Usually that happens on blur, not every keystroke. Does the editor already have infrastructure for this? As an alternative, we can just grab the current data from the outside, but then we wouldn't have visibility of dirty state. We can discuss on Wednesday, but this is obviously a very important part for any integration.

Mikey Stengel added 4 commits April 12, 2024 11:53
* Send editor state updates via onChange via BroadcastChannel - tbd if this is the right approach
* Implement class property next to attribute for direct state manipulation
@hejtful
Copy link
Contributor

hejtful commented Apr 16, 2024

@CodingDive In the current code, there are:

  1. onChange property (which doesn't seem to be used by the editor)
  2. Event broadcasting
    Do we need both of these, or can we remove the onChange prop? And did you plan to add some documentation about the event broadcasting (ideally, you could add it to this PR)?

@CodingDive
Copy link
Contributor Author

@hejtful done here #3658.

The onChange, I see passed down here and used https://github.com/serlo/frontend/blob/staging/packages/editor/src/core/inner-document.tsx#L27. Web component gets the state updates from the editor via onChange but then broadcasts it to the web component consumer via the customEvent.

@yorrd
Copy link

yorrd commented Apr 22, 2024

As discussed in person, need iteration on shadow dom integration because it currently breaks the rendering of CSS due to scoping issues. Detailed feedback on usability will be iterated on after the merge in collaboration with @CodingDive .

Therefore, happy to approve as a preliminary release version for guided internal use.

@CodingDive CodingDive merged commit 2888998 into staging Apr 22, 2024
7 checks passed
@CodingDive CodingDive deleted the feat/wrap-editor-in-web-component branch April 22, 2024 08:43
@elbotho elbotho mentioned this pull request Apr 22, 2024
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.

5 participants