MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 925 forks source link

When utilizing shared mithril components imported from another node package, mithril render and routing do not work as expected #2662

Closed omenking closed 2 months ago

omenking commented 3 years ago

Mithril version:

2.0.4

Browser and OS: Chrome, MacOS and Windows10

Project:

https://github.com/omenking/mithril_issue

Code

https://github.com/omenking/mithril_issue

Expected Behavior and Current Behavior

https://github.com/omenking/mithril_issue

1. Routing

When using m.route.Link within the shared_ui/nav.js the url ends up as follows:

http://localhost:8080/hello#!/world

instead of the expected:

http://localhost:8080/world

2. Rendering

When using m.request within the shared_ui/notifications.js the success function is suppose to trigger a redraw but it does not. Adding an explicit redraw() does nothing with the component.

Steps to Reproduce

I made a video demonstrating the two issues:

https://youtu.be/GIJiAK7eGwc

Context

I am refactoring my large monolithic application into smaller front-end web-apps based on domain. I have created a UI Kit for and common components eg Header, UserDropDown, Notifications to use across these frontends.

9r1nc3w1ll commented 3 years ago

Your render_items function could use Array.map to avoid creating the elements.

I also think that the fetch is not triggering a redraw because you are using it in the contructor method. Try putting the fetch in an oninit lifecycle method and let's see how that goes.

9r1nc3w1ll commented 3 years ago

Yes you need to put the fetch in an oninit what I think is happening is that fetch runs completely before the component is fully rendered by the view method and so it does not pick up the redraw or data changes

cavemansspa commented 3 years ago

@omenking -- nice video / explanation on what you're experiencing. one observation in looking at your example: take a look at https://mithril.js.org/components.html#avoid-fat-components

i'd recommend making your data components (aka models) completely standalone / independent of your view components.

you can use oninit as @omenking suggests, or i prefer to use onmatch when changing routes. you can invoke a request in onmatch for the data required for the route being rendered.

if you have more questions, feel free to hop on the chat channel at: https://gitter.im/mithriljs/mithril.js. usually some helpful people available there.

here's an example using onmatch

omenking commented 3 years ago

@9r1nc3w1ll

Your render_items function could use Array.map to avoid creating the elements.

This codebase is an approximation of my replicated problem, I normally use coffeescript. I just needed to get a working example.

@9r1nc3w1ll

I also think that the fetch is not triggering a redraw because you are using it in the contructor method.

This is not the problem because in my actual codebase I have it in oninit. To prove this is not the case I have updated the sample code to show that it's still a problem.

@cavemansspa

@omenking -- nice video / explanation on what you're experiencing. one observation in looking at your example: take a look at https://mithril.js.org/components.html#avoid-fat-components

I don't think fat components are my issue. This is throwaway code to stage the example.

@cavemansspa

i'd recommend making your data components (aka models) completely standalone / independent of your view components.

The problem is a redraw is not being triggered. Holding state within a Component Class vs an isolated datastore is a subjective choice. The way I like to use mithril is having the Class Component hold state for the generic components, and it's worked for many years. I have build an entire boilerplate on top of it, so it would be very burdensome for me to change my architecture right now.

@cavemansspa

i prefer to use onmatch when changing routes. you can invoke a request in onmatch for the data required for the route being rendered.

I did not show it but in my use case, but then what I'm doing is someone clicks a button and then it triggers data to be loaded. So onmatch would not help resolve this issue, even if it did, it would be a workaround.

cavemansspa commented 3 years ago

Holding state within a Component Class vs an isolated datastore is a subjective choice.

agree @omenking

The way I like to use mithril is having the Class Component hold state for the generic components, and it's worked for many years.

no prob, did not realize you had an existing implementation.

i think i have flems that's reproducing the prob, however i'm not that familiar with the nuances of js classes and the scope of the this binding in the constructors and if / how it's impacting.

let's see if someone else can weigh-in on the issue.

osban commented 3 years ago

@omenking I'm not sure I understand things correctly, but AFAICT the render_items shouldn't be done in the view, and in fact isn't needed at all. Here's an example. No idea why the hashbang shows up...when I mouseover the link it shows correctly.

Edit: I suddenly remembered someone in chat having the same issue, and IIRC it turned out to be a Webpack problem.

orbitbot commented 3 years ago

@barneycarroll identified it in the chat that it's an issue of having multiple separate mithril instances loaded in the same page.

This should probably be fixable by tweaking the webpack configuration.

As an example, if you temporarily use script tags in the head to load a single instance in the app, everything works as expected. See this git diff (copy paste into a text file, and git apply <path.to.file>

diff --git a/my_app/package.json b/my_app/package.json
index 2cbd498..26e46d3 100644
--- a/my_app/package.json
+++ b/my_app/package.json
@@ -19,10 +19,11 @@
     "webpack": "^5.26.3",
     "webpack-cli": "^4.5.0",
     "webpack-dev-server": "^3.11.2",
-    "shared_ui": "/Users/andrewbrown/Desktop/mithril_debug/shared_ui"
+    "shared_ui": "../shared_ui"
   },
   "scripts": {
     "build": "webpack --mode production",
+    "build:dev": "webpack --mode development",
     "start": "webpack serve --mode development",
     "test": "echo \"Error: no test specified\" && exit 1"
   }
diff --git a/my_app/src/index.html b/my_app/src/index.html
index 51291e0..df02c58 100644
--- a/my_app/src/index.html
+++ b/my_app/src/index.html
@@ -3,6 +3,8 @@
   <head>
     <meta charset="UTF-8">
     <title>My App</title>
+    <script src="https://unpkg.com/mithril@2.0.4/mithril.min.js"></script>
+    <script src="https://unpkg.com/mithril-stream@2.0.0/stream.js"></script>
   </head>
   <body>
     <main></main>
diff --git a/my_app/src/index.js b/my_app/src/index.js
index 4192b20..13267f7 100644
--- a/my_app/src/index.js
+++ b/my_app/src/index.js
@@ -1,8 +1,8 @@
-import m from "mithril";
-import stream from "mithril/stream";
 import Nav from 'shared/components/nav'
 import SharedNotifications from 'shared/components/notifications'

+window.stream = m.stream
+
 class Notification {
   view(vnode) {
     return m('.notification', vnode.attrs.item.name)
@@ -82,7 +82,7 @@ class World {
 }

 m.route.prefix = ''
-m.route(document.body,'/hello',{
+m.route(document.body, '/hello', {
   '/hello': Hello,
   '/world': World
 })
diff --git a/shared_ui/components/nav.js b/shared_ui/components/nav.js
index 32561a0..9758cc2 100644
--- a/shared_ui/components/nav.js
+++ b/shared_ui/components/nav.js
@@ -1,5 +1,3 @@
-import m from "mithril";
-
 export default class Nav {
   constructor(vnode){
   }
diff --git a/shared_ui/components/notifications.js b/shared_ui/components/notifications.js
index c99f06d..1b7088e 100644
--- a/shared_ui/components/notifications.js
+++ b/shared_ui/components/notifications.js
@@ -1,6 +1,3 @@
-import m from "mithril";
-import stream from "mithril/stream";
-
 class Notification {
   view(vnode) {
     return m('.notification', vnode.attrs.item.name)
@@ -28,7 +25,6 @@ export default class SharedNotifications {
   success(data){
     console.log('data',data)
     this.items(data)
-    m.redraw()
   }
   render_items(){
     const elements = []
@@ -40,7 +36,7 @@ export default class SharedNotifications {
   }
   view() {
     return m('.notifications',[
-      this.render_items()
+      this.items().map(item => m(Notification, {item}))
     ])
   }
 }
omenking commented 3 years ago

Thanks @orbitbot

I think had read a github issue by @barneycarroll prior and this is where my speculation came about that there were two instances.

As a solution I won't be able to insert mithril into the head because I load mithril through my boilerplate and expose its API around my own. It will also make my serverless deployment pipeline more complicated.

I wonder how to solve this issue hmm.

omenking commented 3 years ago

Talking to @barneycarroll via gitter the problem is likely: Doppelganger package where mithril is being loaded twice because of webpack.

https://rushjs.io/pages/advanced/npm_doppelgangers/

When I have a moment I'll attempt to resolve this and post the solution

omenking commented 3 years ago

I just had to resolve mithril as such and it fixed it

  resolve: {
    extensions: ['.coffee','.js','.json', 'scss'],
    modules: ['node_modules/'],
    alias: {,
      lib        : path.resolve(__dirname, 'javascripts/lib/'),
      'mithril'  : path.resolve(__dirname, 'node_modules/mithril')
    }
  },