ramp4-pcar4 / storylines-editor

An editor for RAMP Storylines
5 stars 13 forks source link

Double-Stuffed Build #380

Closed james-rae closed 1 week ago

james-rae commented 2 weeks ago

Description

A great thing for an Oreo. A bad thing for a Respect Editor.

From what I can tell, the live editor build is bundling the RAMP library twice. Clues:

I suspect that the RAMP Config editor is consuming RAMP, gets bundled & built, pushed to NPM. RESPECT editor then consumes it twice, once via RAMP library, second time via RAMP Config Editor library.

Obviously want to stop this.

Environment

DOMM, Disrespectful on Dev

Possible solution

Some ideas from a non-build-expert.

  1. Looking at this line, can we just stop importing the RAMP library in RESPECT Editor? From what I can tell, we are only importing the CSS file. Would be reeeeeeeeal nice if solution is that easy, since everything that follows it nasty. Biggest gotcha I see here is you can't refresh RAMP in RESPECT without also releasing a new tag on RAMP Config Editor. Which is what the patch number on the version is suited for, but still seems a tad silly.
  2. Change RAMP Config Editor to import RAMP via package.json instead of having a local copy of RAMP build. Then cross fingers and hope RESPECT editor build has tree shaking smart enough to drop the duplicate.
  3. For the above to work, a strategy will need to be in place to ensure that RAMP Config Editor and RESPECT Editor are always pointing to the same version of RAMP on NPM. This sort of ties in with the gotcha mentioned in the first idea.
  4. Make the RAMP Config editor source a subfolder of RESPECT editor. Now they can share the same import. Lazy but effective. Breaks the ability to grab RAMP Config editor as a single unit for other purposes.
  5. Start investing in deeply understanding builds and bundlers, which is hurting us everywhere (https://github.com/ramp4-pcar4/ramp4-pcar4/issues/1528).
mohsin-r commented 2 weeks ago

More half baked thoughts from a build noob.

Looking at this line, can we just stop importing the RAMP library in RESPECT Editor?

This is probably my fault for picking the same name, but this is actually not importing the RAMP library. It’s importing the config editor’s createInstance() which mounts an instance of the config editor onto the RESPECT editor. TLDR is we definitely need that import.

you can't refresh RAMP in RESPECT without also releasing a new tag on RAMP Config Editor.

Previous build issues have already led to the tags looking pretty silly on that package.

Change RAMP Config Editor to import RAMP via package.json instead of having a local copy of RAMP build. Then cross fingers and hope RESPECT editor build has tree shaking smart enough to drop the duplicate. For the above to work, a strategy will need to be in place to ensure that RAMP Config Editor and RESPECT Editor are always pointing to the same version of RAMP on NPM. This sort of ties in with the gotcha mentioned in the first idea.

I think this is definitely worth a shot. Updating to the RAMP npm package should not be too much of a hassle in the config editor.

james-rae commented 2 weeks ago

Sad, but thanks for the clarification. Calling it createInstance() is fine, since it makes sense WRT the library. The storylines code can be improved by doing this.

import { createInstance as createRampEditorInstance } from 'ramp-config-editor_editeur-config-pcar';
...
this.rampEditorApi = createRampEditorInstance(this.$refs.editor, conf)
james-rae commented 2 weeks ago

Still curious if first option can work. I can't find anywhere in the respect editor where it makes a new RAMP instance. The RAMP config editor does, but it's using the RAMP bundled inside it.

<storylines-content> in preview.vue seems to be what launches the storyline preview, and in classic Vue fashion you can't search for storylines-content or StorylinesContent and get where it's source from.

Edit: think i've found it, Respect editor also imports ramp-storylines_demo-scenarios-pcar, and it imports RAMP. Because it's using the same version of RAMP as the package.json in Respect Editor, I'm assuming the build is smart enough to not add it 3 times.

The <storylines-content> is the thing from inside ramp-storylines_demo-scenarios-pcar, and that's where the preview gets spawned and RAMP instances instantiated.

So I suspect easy idea 1. won't do anything as the library will still come in via the Storyline Viewer library. But it does give a bit of hope that aligning the Ramp Config Editor version (and having it use a package.json import instead of local file) may allow a share of everything.

mohsin-r commented 2 weeks ago

I tried updating the config editor to the RAMP npm package with the same version as the storylines repo (4.6.0). Sadly the build file was still about 26-27MB.

james-rae commented 2 weeks ago

Sad times indeed.

If options 4 or 5 above doesn't provide a nice solution, some more extreme alternatives:

  1. Make alternate builds of Storylines Viewer and RAMP Config Editor that require RAMP to be installed (using the bundled library that creates window.RAMP var). Use the current bloated approach for Respect development, but when time for "prod build", need to swap SLViewer and RCEditor, and add the bundled Ramp script to Respect Editor page. Real messy, but will work. Not sure which implementation would be worse: doing by hand or attempt to automate the nonsense.
  2. Slightly different alternative to above: Have Storylines Viewer and RAMP Config Editor get passed the Ramp createInstance method for use. Still messy. Easier to do in RCEditor since it has its own createInstance. The Storylines Viewer doesn't have that, it provides a Vue component that gets magically linked in via Respect Build. I don't know enough to know what's feasible.
  3. Look into the "Chunked RAMP on a CDN" ideas from https://github.com/ramp4-pcar4/ramp4-pcar4/issues/2155#issuecomment-2233596768. Major change but may also improve page startup times of Storylines & Respect Editor if it works.
yileifeng commented 2 weeks ago

Did not see this earlier, but as James suspected removing ramp-pcar dependency from the editor does nothing. Currently, it is not utilized at all since both RAMP editor and Storylines viewer have its own RAMP dependencies and everything still functions even when removing ramp-pcar from editor. I also got confused about the createInstance import so will make the alias suggestion above in the editor when importing from RAMP config editor.

Will attempt some local npm link testing to test effectiveness, but two additional thoughts tied to new ideas 1-2 by James above that might be worth attempting (would say option 4 in issue is more of a last resort, avoid if possible):

james-rae commented 2 weeks ago

would say option 4 in issue is more of a last resort

I don't think its even viable anymore, unless the Storylines Viewer library can export the RAMP API for Ramp Config Editor to use.

yileifeng commented 2 weeks ago

Did some testing and managed to get the editor build to ~17MB after reducing the plugin builds of both the Config Editor and Storylines Viewer individually by externalizing the ramp-pcar dependency. Seems like should resolve the issue of editor bundling NPM RAMP twice. With the shrunken plugin dist builds, I used npm pack to test both projects in the editor locally and RAMP instances were behaving normally when running sanity checks.

Here are the steps I took:

  1. In the vite.config.ts of both plugins, I added the following rule under rollupOptions: external: ['ramp-pcar'] (ensure this is under rollupOptions directly and not rollUpOption > output)

  2. (might not need this step) In the package.json of both plugins, I hard set the ramp-pcar version to be "4.6.0" instead of "^4.6.0", moved it to devDependencies instead of direct dependencies, and also added it under peerDependencies to ensure the consuming project (Storylines Editor) is required to install ramp-pcar.

  3. Re-built the Storylines Viewer for plugin with npm run build-plugin, yielding a ESM output file of 1.03 MB

  4. Re-built the Config Editor for plugin with npm run build:lib, yielding a ESM output file of 0.9 MB

In contrast, both of these plugin builds were over 16 MB when being imported in the Storylines editor currently.

Finally, I just packed these builds and set the editor's dependencies of "ramp-config-editor_editeur-config-pcar" and "ramp-storylines_demo-scenarios-pcar" to the .tgz files with relative paths to the other two projects and re-installed + ran the editor to get the results. If this fix is acceptable, we would just need to add a 5th step above to re-publish both projects to NPM for glory.

james-rae commented 2 weeks ago

Nice solution. How does this impact the RAMP Config editor and Storylines Viewer as stand-alone items? Like when we build the Storylines site, will it need extra steps to make it work? Cuz if I'm understanding the above it won't have RAMP without extra steps.

Feels like we're making these two items dependencies with missing parts that consumer needs to provide, instead of stand-alone units of code. But if we can't find a saner solution, might need to consider.

yileifeng commented 2 weeks ago

That's a good point, it should not impact the projects building on its own IF step 2 above is unnecessary as we have separate Vite config options (where we specify the ramp-pcar being excluded from bundle) when building for NPM plugin. However, I think if ramp-pcar is placed in devDependencies running a standard npm run build might fail (maybe there is a workaround for that). I will try testing if keeping ramp-pcar in regular dependencies will work for plugin, since I was attempting a bunch of steps together in hopes of finding a solution and if this still works we should be good.

yileifeng commented 2 weeks ago

The config editor + Storylines viewer plugin NPM builds were updated with the Vite external rule and published. The dist build size can be found below:

https://www.npmjs.com/package/ramp-config-editor_editeur-config-pcar?activeTab=code

https://www.npmjs.com/package/ramp-storylines_demo-scenarios-pcar?activeTab=code

Installing the new versions of each library in the editor retains all RAMP functionality and can be tested in this PR. The main.js file is ~17MB. Storylines demo also runs fine as a standalone project which can be tested here. Not sure if there is a demo for the config editor but it follows the same build steps. RAMP was also upped to latest 4.8.0 for both projects.