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

Upgrade frontend build stack for CMS5 #1312

Closed
7 tasks done
maxime-rainville opened this issue Jun 25, 2022 · 7 comments
Closed
7 tasks done

Upgrade frontend build stack for CMS5 #1312

maxime-rainville opened this issue Jun 25, 2022 · 7 comments

Comments

@maxime-rainville
Copy link
Contributor

maxime-rainville commented Jun 25, 2022

Our current JS/CSS build stack is horribly outdated. We need to refresh this to a more modern stack.

Acceptance criteria

  • All our JS dependencies are supported
  • Default to running Node v18
  • Where an LTS version of a dependency is available, this is preferred to a shorter lived version
  • All our module with front-end clients are upgraded to work with the new stack
  • Reassess whether there's value swapping to NPM instead of yarn
    • Decision: They're effectively equivalent. Keeping yarn means we don't have to rewire our brains so stick with that for now. In the future we can consider if some faster alternative is worthwhile, since npm and yarn are notoriously slow, but for now that's not a big enough issue to worry about
  • Reassess whether there's value in changing the way to build our CSS
    • Decision: Not at this stage.
  • This changes is not back ported to CMS4 as part of this card

Note

  • If it's possible to back-port this build stack to CMS4, we should consider it. But it's not the primary purpose of this card.
  • Because of the weirdness of our current stack, it might be more cost affective to scrap our current build system and swap to something more generic. But we are not sure yet. We are not very attached to the current build system. Who ever picks up this card should make an initial assessment before proceeding either way.
  • We are 3 majors behind Webpack.
  • We might want to use the clean option to ensure our dist/ directory never has old unused files.

CI

All PRs are included in this behat-only CI run on installer:
https://github.com/creative-commoners/silverstripe-installer/actions/runs/3709651281

PRs

@GuySartorelli
Copy link
Member

Probably too early to adopt for now, but worth keeping an eye on: https://bun.sh/

@GuySartorelli
Copy link
Member

GuySartorelli commented Nov 27, 2022

I've done a large portion of the work upgrading dependencies now - here's a list of current regressions I've found that need to be fixed before anything can be merged in (i.e. things that are broken):

Critical regressions (need to fix first)

  • CSS can't be built
    • Maybe use a different tool for that - no need for complex webpack config
  • Copy output doesn't work
    • This one is me being stubborn, so I won't spend much effort on it - I want to have the copying done as a separate step, rather than lumped in with the javascript transpilation.
  • Page selection in link to page modal in tinymce
    • Actually just tree dropdown in general
  • tinymce modal closing (complains that I'm trying to create root when root already exists or some such)
    • I haven't rebased since the jQuery 3 stuff was merged in so maybe it's related to that somehow?
  • Uploading in a WYSIWYG (uploads, but then screams and kicks out of the upload modal)
    • Also selecting existing file in WYSIWYG - probably the same root cause
    • Seems to work fine for link to file
  • Validation errors throw away changes
  • Form submission causes tab to freeze (infinite loop somewhere?)
    • Seen on validation error and on successful create. Happened on publish but the freeze wasn't immediate so it's not a blocking loop if there's a loop at all.
  • Archive action in ... of gridfield (no errors, no network event fires... maybe a CSS selector issue at its heart?)
  • WYSIWYG add image doesn't open folders - closes modal and plenty of console errors.
  • Assets admin table view doesn't work - no console errors.

High impact regressions (could theoretically spawn separate cards for these, but should be fixed before stable launch)

  • A lot of the styling looks different from how it used to (in a bad way)
  • React no longer has "You have unsaved changes" confirm dialog when navigating (see CMS 5: React context no longer has "You have unsaved changes" confirm dialog #1409)
  • React breadcrumbs (and likely other links) show e.g. "/assets" instead of "/admin/assets" (but still navigate correctly within react context)
    • This is a result of updates to routing which doesn't require (or allow) the parent route to be included anymore - i.e. because /admin is the parent route, the children must be routed as assets/etc - which means they work correctly to react, but the URL is then incorrect for middle mouse clicking etc.
    • Try this before anything subsequent: Can I just use <Link> components instead of <a> tags?
    • Likely just need to separate out href URL and react-routed URL - but I think we just let react router handle link clicking events directly.... so we may need to find out how to override that
    • Ideally let link href include the full URL and then on click we strip out the prefix from any parent routes
      • This ensures that links will just work without us having to directly control them (i.e. links added via project-specific code will work without alteration) and reduces the chance of uncaught regressions.
    • Alternatively let link href include the full URL but have a separate data-react-href to pull the already-stripped routing path on click
  • Asset admin can't collapse ... options after expanding
  • Some CSS styles broken (just needs to be rebuilt for the most part - but may need some changes like bootstrap namespacing on some class names)
  • It's hard to delete images in the WYSIWYG (see CMS 5: Cannot delete image in WYSIWYG with delete button #1410)
  • Many yarn test failures

Lower priority regressions (could be separate cards to be triaged in their own right)

Also present in CMS 4 (create new cards for these)

@GuySartorelli
Copy link
Member

NOTE: I have spun off https://github.com/silverstripeltd/product-issues/issues/644 for the kitchen sink. This card is only for core modules.

@GuySartorelli
Copy link
Member

NOTE: I will do the changelog as a part of #1318

@emteknetnz emteknetnz self-assigned this Dec 13, 2022
@emteknetnz
Copy link
Member

emteknetnz commented Dec 13, 2022

@GuySartorelli - I'm happy with the code in these PRs, two things we should do before merging

  1. Fix all the globally broken end to end tests on 5 first, then rerun all the end to end in there PRs to see if there were any regressions caused by these PRs. Link PRs on this issue. As discussed it's probably OK to merge with broken end to end tests now and then fix in a subsequent issue, though we at least first need data to see if these PRs are what caused regressions

  2. Do a combined CI run with all the PRs here using using the composer_install option

Instructions:

  • Fork silverstripe-installer - point origin at your personal github account or creative-commoners
  • Update .github/workflows.ci.yml and add composer_install: true
  • Update composer.json repositories + requires to use the branches linked in the issue
  • Update composer.json set config.platform.php to "8.1"
  • Run composer update --no-install to get a composer.lock file
  • Commit the updated composer.json + composer.lock and push to your personal-account or creative-commoners
  • Do not create a PR - just see the run in the actions tab
  • Link to the action run in this issue

@GuySartorelli
Copy link
Member

@emteknetnz I've added a full installer CI run in the issue description now that our behat tests are running again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants