Rewrote the asset build pipeline #1474
Conversation
|
Note: for the npm tests to pass the |
| import LiveSocket from "phoenix_live_view/live_socket" | ||
| export { | ||
| LiveSocket | ||
| } |
There was a problem hiding this comment.
| import LiveSocket from "phoenix_live_view/live_socket" | |
| export { | |
| LiveSocket | |
| } | |
| export { default as LiveSocket } from "phoenix_live_view/live_socket" |
There was a problem hiding this comment.
We are not exporting LiveSocket as default
There was a problem hiding this comment.
Actually, I take that back. There appears to be some inconsistency in the existing JS. LiveScocket is being exported as default and as a named export: https://github.com/phoenixframework/phoenix_live_view/blob/master/assets/js/phoenix_live_view.js#L3018
The generated app.js from Phoenix assumes named export: https://github.com/phoenixframework/phoenix/blob/master/installer/templates/phx_assets/app.js#L18
The documentation in Elixir assumed default export: https://github.com/phoenixframework/phoenix_live_view/blob/master/lib/phoenix_live_view.ex#L178
but that code snippet is not consistent with the app.js code: https://github.com/phoenixframework/phoenix/blob/master/installer/templates/phx_assets/app.js#L18
I am OK adding the default export, I just need @chrismccord to weigh in on what the intention is here. I suspect somewhere along the way this detail got confused and we ended up with both ways.
There was a problem hiding this comment.
If we want we can deprecate the default export by adding a console.log message indicating that default is deprecated in favor of the named import, then align all documentation around the named import.
There was a problem hiding this comment.
@bcardarella that line should export the default export as a named export.
Basically the same you wrote in one statement.
There was a problem hiding this comment.
Ah I see, that's interesting. I didn't know you could do both at once. Let me get feedback from @chrismccord on the default vs named export preference. If a change is going to be made now would be the time. If no change will be made I'll happily change to your suggestion.
There was a problem hiding this comment.
I spoke with @chrismccord and we're going to just go with named export and not have a default export.
|
|
||
| LiveImgPreview: { | ||
| mounted(){ | ||
| this.ref = this.el.getAttribute("data-phx-entry-ref") |
There was a problem hiding this comment.
This PR is a great resource for learning how to separate concerns. Thank you.
I'm curious, is this attribute not constantised because it's the only place it's used?
There was a problem hiding this comment.
This particular LOC wasn't changed when extracted into the class-per-file style. After this is merged and vetted we can probably take a look at some refactors.
|
Merged manually in master ❤️❤️❤️🐥🔥 |
|
What is the reason that the production source maps were removed (introduced in #1385)? |
See phoenixframework/phoenix#4337 for details
A few differences from the Phoenix PR are due to this repo using Jest and not Mocha to test the JavaScript. Jest has been updated to latest and the previously existing settings in
package.jsonhave been moved over to thejest.config.jsfile