-
Notifications
You must be signed in to change notification settings - Fork 11
New injections for forked elm packages #65
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
base: lamdera-next
Are you sure you want to change the base?
Conversation
| function sendToApp(msg, viewMetadata) | ||
| { | ||
| if (buriedTimestamp !== null) { | ||
| const elapsed = Date.now() - buriedTimestamp; | ||
| // This tries to turn `HomePageMsg (WeatherWidgetMsg (WeatherReportReceived WeatherReport))` | ||
| // into `"HomePageMsg WeatherWidgetMsg WeatherReportReceived"`. | ||
| // The idea is that if the timeout for forwarding messages isn't enough, we want to know what | ||
| // message somebody used that took even longer, but without reporting the entire msg. | ||
| // Note that in `--optimize` mode, the above string would become something like `"1 3 6"`, | ||
| // but it's better than nothing. | ||
| let msgName = '(unknown message)'; | ||
| if (msg.$) { | ||
| msgName = msg.$; | ||
| let current = msg; | ||
| while (current.a && current.a.$ && !current.b) { | ||
| current = current.a; | ||
| msgName = msgName + ' ' + current.$; | ||
| } | ||
| } | ||
| bugsnag.notify(new Error('Got message ' + elapsed + ' ms after app was buried: ' + msgName)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@supermario Has this ever happened in Bugsnag? Was it worth it? I have a feeling we don’t need this anymore, so I haven’t ported it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked and nope, looks like we've never gotten this bugsnag yet... but this feels like one of those things that's not that big a deal until it happens, and then not having it will be very confusing. Perhaps just being overly paranoid. Happy to leave it for now 👍
Conflicts: extra/Lamdera/Injection.hs
| [submodule "extra/package-replacements/elm/virtual-dom"] | ||
| path = extra/package-replacements/elm/virtual-dom | ||
| url = git@github.com:lydell/virtual-dom.git | ||
| branch = hot-reload-stop | ||
| [submodule "extra/package-replacements/elm/browser"] | ||
| path = extra/package-replacements/elm/browser | ||
| url = git@github.com:lydell/browser.git | ||
| branch = lamdera | ||
| [submodule "extra/package-replacements/elm/html"] | ||
| path = extra/package-replacements/elm/html | ||
| url = git@github.com:lydell/html.git | ||
| branch = safe | ||
| [submodule "extra/package-replacements/elm/core"] | ||
| path = extra/package-replacements/elm/core | ||
| url = git@github.com:lydell/core.git | ||
| branch = lamdera |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve pointed these to my repos while developing, but before merging I think it makes sense to have forks in github.com/lamdera/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not super concerned to fork, but was there a practical reason for it? Like are you worried we might end up needing a Lamdera specific variant, or just a cheap hedge to make now just in case?
Summary
This adds https://github.com/lydell/elm-safe-virtual-dom to Lamdera, simplifies the internal implementation and allows for “proper” hot reloading during development in the future.
Details
I’m working on adding hot reloading and stopping of apps directly to elm/core. Read about it here:
https://github.com/lydell/core/blob/hot-reload-stop/javascript-interface.md#appstop
Branches:
As you can see above, elm/core has a lamdera branch:
elm/browser also has a lamdera branch:
Browser.Navigation.Key.Benefits of this PR:
lamdera livein a future PR.TODO
Installing the forked packages
Here’s the plan: Start out simple and make things more complicated in the future if needed.
ByteStringof alternate file content. Used to do the replacements.Validate versions
In the TemplateHaskell-created map, each elm/* package that we replace specifies a version. That’s the version that the forked package was based on. In elm.json, there will still be lines like
"elm/core": "1.0.5". What if the version in elm.json and the “fork version” do not match?First, let’s go through the scenarios where the fork version is higher than the version specified in elm.json:
1.xversions exists of all elm/* packages we replace. But we could code it to be a hard error.1.0.xversions exists of all elm/* packages we replace. But we could code it to be a hard error."elm/virtual-dom": "1.0.3"in their elm.json, and haven’t bothered updating to the recent 1.0.4 version. If we made this a hard error, it would be annoying for lots of people. I think it’s better to simply allow the fork patch version to be greater than specified. If we want to, we could inform about this somehow (log message, or maybe the comments in the compiled JS shown below is enough?).Then, let’s go through the opposite scenarios, where the fork version is lower than the version specified in elm.json:
"elm/virtual-dom": "1.0.5"in their elm.json, I think it should be a hard error, informing them that you can’t go above 1.0.4 with this release of the Lamdera compiler. (Silently using 1.0.4 anyway would be misleading, leading to a false sense of security.) They need to wait for a new Lamdera compiler version that has pulled in the security fix.lamdera inituses the known versions of elm/virtual-dom etc.In the compiled JS, it might be nice adding comments for debugging:
Replacements
When reading files from disk in
ELM_HOME, check if the file is in the replacement maps. If so, go with that file content instead of the actual files.This requires two changes to caches:
ELM_HOMEthere areartifacts.datfiles, which Lamdera already callartifacts.x.dat. Now, they’ll be called for exampleartifacts.lamdera-1.3.3-0.19.1.dat. This is because we can’t trust an artifacts file made by a previous Lamdera version. It might have had different package replacements.elm-stuff/there’s usually a0.19.1folder with artifacts. Now there will beelm-stuff/lamdera-1.3.3-0.19.1/instead. This is again because cached stuff from a previous Lamdera version cannot be trusted – they might contain the wrong package replacement code. (We don’ use the same approach forELM_HOME–~/.elm/lamdera-1.3.3-0.19.1/– because IDE:s expect packages to be in~/.elm/0.19.1/.)Pros and cons
The benefit of having hard-coded (TemplateHaskell) maps of replacements is simplicity. We know that the provided packages are going to work with Lamdera and hot reloading.
The downside is that we need to make a new compiler release if there’s a hotfix in one of the replaced elm/* packages. However, that happens so infrequently that it feels safe to try out this approach.
How hot reload could work
Here’s how hot reload could work. I think we can do it as a follow-up PR, but I’m also open for doing it in this PR if somebody wants to pair on it.
The last commit of #29 adds two interesting things:
maintype hasn’t changed.) It does that by adding a script tag pointing to the URL/_x/js./_x/js.If the endpoint returns JavaScript like this, hot reload should Just Work: