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

Frontend Framework Rewrite #1821

Closed
61 of 65 tasks
dsevillamartin opened this issue Jul 12, 2019 · 18 comments
Closed
61 of 65 tasks

Frontend Framework Rewrite #1821

dsevillamartin opened this issue Jul 12, 2019 · 18 comments
Assignees
Milestone

Comments

@dsevillamartin
Copy link
Member

dsevillamartin commented Jul 12, 2019


Obsolete Todos / Notes from original approach

See #1950.

  • Update to Mithril v2
  • Integrate TypeScript
    • Requires some kind of typings package, @flarum/types
    • Easily catch errors on compile time
  • Replace moment with alternative such as dayjs
  • Replace jQuery with alternative such as zepto
  • Drop bootstrap, replace with component alternatives where necessary
    • Replace modal plugin with micromodal
    • Replace tooltip plugin with tooltip.js
      • feel free to suggest lighter alternatives for tooltip.js
    • Fix some weird incompatibilities with dropdown plugin or replace
    • Upgrading to bootstrap v4 (Upgrade twitter bootstrap (js) to version 4 #1589) changed a bunch of stuff, would require changes anyway
    • Bootstrap requires jQuery, increasing bundle size
  • [Tags] Replace html5sortable with dragula
    • Includes mobile support and has a smaller minified size

Important thing to note is to not change too much, we already had a big change with webpack.

@dsevillamartin
Copy link
Member Author

Some work has been done on the ds/frontend-framework-rewrite branch.

Bootstrap's affix plugin, moment, jquery, jquery.hotkeys have all been replaced with alternatives.

  • moment has been replaced with dayjs, but kept globally so language packs require minimal to no changes to continue working
  • jquery replaced with zepto, some issues with compatibility and missing features (specially animations, hopefully tackling that right now)
  • jquery.hotkeys replaced with mousetrap, popular + easy to use

Finding alternatives for the rest of the bootstrap plugins has been a bit hard, so no progress has been made on any others yet. Mainly testing + failing. However, we might be able to use some of them with Zepto (some patches needed, but not hard).

Haven't touched Mithril v2 or TypeScript yet (excluding some testing done with both previously).

The built dist files have gone down to 268K (forum) and 188K (admin) from 356K and 276K respectively.

@yulei745
Copy link

Why use some outdated libraries such as: zepto, jump.js, so the library has not been updated for a long time.

@matteocontrini
Copy link
Contributor

I'm not convinced about Zepto either. What's the reason behind that choice? Is it only for the reduced size of the library? I'm not seeing any other advantage, as it seems it hasn't been updated for 3 years, and others have dropped it for performance reasons. Plus I think it could create issues with extensions that rely on jQuery being available.

@dsevillamartin
Copy link
Member Author

@yulei745 @matteocontrini

In a best case scenario, we wouldn't need jQuery at all to do what we want. However, as we probably don't want to reinvent the wheel, we use plugins that chose jQuery for easier coding instead of native implementations. By switching to Zepto, we can minimize the impact jQuery has on the bundle size and maintain compatibility with some jQuery plugins with some patches to Zepto (see js/src/common/utils/patchZepto.js).

I don't think performance is much of a concern here as jQuery is only used when necessary (and could probably be removed completely with some work). And we don't really take advantage of many of the features jQuery has to offer, most of which can be done better natively (http://youmightnotneedjquery.com/), apart from IE 11 support.

Extensions that rely on jQuery being available will still probably work, unless they use complex selectors not supported by Zepto. Besides, this is a rewrite, and while I am trying to make everything as backwards-compatible as possible, there will be breaking changes. We are on beta versions after all 🤷‍♂.

And a library doesn't necessarily have to be recent to work well. The issues that jump.js has in its repository really don't concern our use case, and mostly seem to be feature requests anyhow. I tested a few smooth scrolling methods before finding one that worked well for Flarum's use case.

Hopefully that answered some of your questions 🙂.

@yulei745
Copy link

I am looking forward to updating to mithril.js v2, but I don't know what I can do.

@dsevillamartin
Copy link
Member Author

@yulei745

I don't know what I can do.

Do you mean to help out with the development, or in terms of extensions ? This isn't a priority, so if you do want to help with Flarum development, please do look at issues in the beta 10 milestone 🙂.

@dsevillamartin
Copy link
Member Author

Some more work has been done.

  • Bootstrap modal plugin replaced with micromodal
  • Bootstrap tooltip plugin replaced with tooltip.js
    • wasn't working for some reason
    • don't love using tooltip.js as it relies on popper.js, which is a very advanced library compared to our needs, meaning bigger bundle size (+20kb)
  • Fixed issues with Boostrap dropdown plugin
    • works just fine now + is small

Build dist files have gone up by 20K each, now 288K (forum) and 208K (admin).

@luceos luceos added this to the 0.1 milestone Sep 27, 2019
@dsevillamartin
Copy link
Member Author

I have started on Mithril v2 and Typescript. The best way to do this, after a few failures, is to start from scratch and bring in existing JS files one by one - i.e. convert files to typescript & mithril v2 as they are brought in. Thanks to @clarkwinkelmann for his help during this as well.

We'll probably want to make our Component class as close to Mithril's components, so those docs can be followed. However, we have introduced a few improvements such as:

  • this.props continues to exist
    • this was done to allow for continued use of initProps and prevent needing to pass vnode/vnode.attrs to every single function
    • developers know how to use it
  • Component.component remains
    • Mithril v2 changed, so now a component's constructor has to accept the vnode instead of simply the attributes/props
    • can use either JSX (<Component prop="value" />) or .component call (Component.component({ prop: 'value' }))
    • calls m under the hood, but its prettier :)
  • m.prop stays, setting it to mithril/stream to keep old functionality
  • m.withAttr manually added, as it was removed from mithril but is a useful helper

Some different things that we've decided not to make backwards compatible / keep the old structure for:

  • init and config have now been replaced with mithril's new lifecycle hooks (oninit, oncreate, onbeforeupdate, onupdate, onbeforeremove, and onremove), see https://mithril.js.org/lifecycle-methods.html for more info on them
  • m.route has changed
  • and probably more

On the side of TypeScript, I've tried to make it so developers can prevent errors by making component props a type (optional). Sadly, it doesn't work with .component as that's a static method, but it works with this.props. It's not very pretty, but it is optional so 🤷‍♂. Feedback on this welcome.

The types & interfaces do not take up any compiled dist space (they appear as empty lines) so that's great 🙂.

export type ComponentProps = {
  children?: Mithril.Children,

  className?: string;
}

export default class Component<T extends ComponentProps = any> {
  protected props = <T> {};
}
export interface ButtonProps extends ComponentProps {
  title?: string,
  type?: string,
  icon?: string,

  loading?: boolean,
  disabled?: boolean,
  onclick?: Function
}

export default class Button<T extends ButtonProps = ButtonProps> extends Component<T> {}

@dsevillamartin
Copy link
Member Author

I have pushed the code I have so far to ds/frontend-framework-rewrite-mithril and flarum-webpack-config@ds/frontend-framework-rewrite.

There's an old folder in js that just contains the plain JS files from the frontend framework rewrite (w/o mithril v2 & typescript). I just use it when moving files to TypeScript.

I have added a markdown file to the root of the branch that contains some of the changes that have been made to the class structures & methods (exlcluding mithril v2 and typescript).

Haven't set up typings yet for flarum/core (and definitely not extensions 🙈), including thinking of how they will be generated, published, etc... and the global shims are a bit of a mess, at least with PhpStorm not liking them but recognizing them (???, I don't know TypeScript typings don't do this to me)

This is basically what I have right now.
img

I'll probably configure prettier at some point to fix the atrocious linting mistakes I'm committing when copy pasting coding.

Dist files are currently 196K (forum) and 132K (admin). Of course, it barely includes any of the classes necessary for Flarum to function like a forum. 😁

@dsevillamartin
Copy link
Member Author

dsevillamartin commented Mar 21, 2020

Progress on the Mithril rewrite, excluding components / utils / helpers that still have to be copied over.

Outdated checklist removed. See OP for recent updates. /Franz

@askvortsov1
Copy link
Sponsor Member

askvortsov1 commented May 9, 2020

As per today's kickoff meeting, here's the proposed roadmap for the remainder of this rewrite. The goal here is to have maximally modular PRs, so that history is as clean as possible. Plan is as follows:

Outdated checklist removed. See OP for recent updates. /Franz

@tankerkiller125
Copy link
Contributor

A possible animation library replacement would be http://velocityjs.org/

@dsevillamartin
Copy link
Member Author

dsevillamartin commented May 17, 2020

And instead of Zepto, cash (or cash-dom in NPM) seems like a better, more updated alternative.

@tankerkiller125
Copy link
Contributor

See #2255 for Mithril 2 updates

@askvortsov1
Copy link
Sponsor Member

askvortsov1 commented Aug 8, 2020

Consider replacing classList with https://www.npmjs.com/package/classnames?

@luceos
Copy link
Member

luceos commented Aug 28, 2020

Can someone distill every todo from this issue (and other issues) for the rewrite into the OP of this issue and include this checklist to signify rewriting bundled extension progress:

- [ ] akismet
- [ ] approval
- [ ] auth-facebook
- [ ] auth-github
- [ ] auth-twitter
- [ ] bbcode
- [ ] embed
- [ ] emoji
- [ ] flags
- [ ] likes
- [ ] lock
- [ ] markdown
- [ ] mentions
- [ ] pusher
- [ ] statistics
- [ ] sticky
- [ ] subscriptions
- [ ] suspend
- [ ] tags

@franzliedke
Copy link
Contributor

What I did:

  • Remove outdated checklists from other comments
  • Combine the relevant checklists (and your list, thanks!) into a new one in the OP
  • Try to link relevant PRs
  • Tick boxes where appropriate to the extent of my knowledge
  • Suggest deferring a few items that would further blow up the size of this release

@franzliedke
Copy link
Contributor

I've started ticking the boxes for the extensions where the Mithril 2.0 upgrades are ready to be merged. Note that we will only merge these once the upgrade in core has been merged.

franzliedke added a commit that referenced this issue Sep 18, 2020
- Update libraries
- Add Typescript typings for Mithril
- Rename "props" to "attrs"
- New lifecycle hooks
- Other mechanical changes, following the upgrade guide
- Remove some of the custom stuff in our Component base class
- Introduce "fragments" for non-components that control their own DOM
- Remove Mithril patches, introduce a few new ones

Challenges:
- Behavior of links to current page changed in Mithril
- Native Promise rejections are shown on console when not handled
- ...

Refs #1821.

Co-authored-by: Alexander Skvortsov <sasha.skvortsov109@gmail.com>
Co-authored-by: Matthew Kilgore <tankerkiller125@gmail.com>
Co-authored-by: Franz Liedke <franz@develophp.org>
@askvortsov1 askvortsov1 self-assigned this Sep 18, 2020
franzliedke added a commit that referenced this issue Sep 18, 2020
- Update libraries
- Add Typescript typings for Mithril
- Rename "props" to "attrs"
- New lifecycle hooks
- Other mechanical changes, following the upgrade guide
- Remove some of the custom stuff in our Component base class
- Introduce "fragments" for non-components that control their own DOM
- Remove Mithril patches, introduce a few new ones

Challenges:
- Behavior of links to current page changed in Mithril
- Native Promise rejections are shown on console when not handled
- ...

Refs #1821.

Co-authored-by: Alexander Skvortsov <sasha.skvortsov109@gmail.com>
Co-authored-by: Matthew Kilgore <tankerkiller125@gmail.com>
Co-authored-by: Franz Liedke <franz@develophp.org>
askvortsov1 added a commit that referenced this issue Sep 24, 2020
* Update frontend to Mithril 2

- Update Mithril version to v2.0.4
- Add Typescript typings for Mithril
- Rename "props" to "attrs"; "initProps" to "initAttrs"; "m.prop" to "m.stream"; "m.withAttr" to "utils/withAttr".
- Use Mithril 2's new lifecycle hooks
- SubtreeRetainer has been rewritten to be more useful for the new system
- Utils for forcing page re-initializations have been added (force attr in links, setRouteWithForcedRefresh util)
- Other mechanical changes, following the upgrade guide
- Remove some of the custom stuff in our Component base class
- Introduce "fragments" for non-components that control their own DOM
- Remove Mithril patches, introduce a few new ones (route attrs in <a>; 
- Redesign AlertManagerState `show` with 3 overloads: `show(children)`, `show(attrs, children)`, `show(componentClass, attrs, children)`
- The `affixedSidebar` util has been replaced with an `AffixedSidebar` component

Challenges:
- `children` and `tag` are now reserved, and can not be used as attr names
- Behavior of links to current page changed in Mithril. If moving to a page that is handled by the same component, the page component WILL NOT be re-initialized by default. Additional code to keep track of the current url is needed (See IndexPage, DiscussionPage, and UserPage for examples)
- Native Promise rejections are shown on console when not handled
- Instances of components can no longer be stored. The state pattern should be used instead.

Refs #1821.

Co-authored-by: Alexander Skvortsov <sasha.skvortsov109@gmail.com>
Co-authored-by: Matthew Kilgore <tankerkiller125@gmail.com>
Co-authored-by: Franz Liedke <franz@develophp.org>
@askvortsov1 askvortsov1 unpinned this issue Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants