mithril.js icon indicating copy to clipboard operation
mithril.js copied to clipboard

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

Open omenking opened this issue 3 years ago • 10 comments

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.

omenking avatar Mar 19 '21 03:03 omenking

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 avatar Mar 19 '21 06:03 9r1nc3w1ll

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

9r1nc3w1ll avatar Mar 19 '21 06:03 9r1nc3w1ll

@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

cavemansspa avatar Mar 19 '21 13:03 cavemansspa

@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.

omenking avatar Mar 19 '21 14:03 omenking

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.

cavemansspa avatar Mar 19 '21 16:03 cavemansspa

@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.

osban avatar Mar 19 '21 16:03 osban

@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/[email protected]/mithril.min.js"></script>
+    <script src="https://unpkg.com/[email protected]/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}))
     ])
   }
 }

orbitbot avatar Mar 19 '21 17:03 orbitbot

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 avatar Mar 19 '21 19:03 omenking

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 avatar Mar 19 '21 19:03 omenking

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')
    }
  },

omenking avatar Mar 19 '21 22:03 omenking