-
Notifications
You must be signed in to change notification settings - Fork 9
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
Compatibility with dynamic imports #7
Comments
Hey @lmartins! Good question, thanks for posting. I've never done dynamic imports with this library, so I'm not sure. Can you provide an example of how you're using it? |
Hey @estrattonbailey, thank you for looking into this. The way I normally use the library is by passing an array of component wrappers, as such:
Each component in this format returns the The components definition would be pretty similar:
But what this FooterGallery holds is a promise as detailed here: So I picoapp would need to determine the component initialisation based on whether the returned value would be a promise or a standard constructor? |
Sure! I'll give it a try.
…Sent from my iPhone
On 11 Sep 2019, at 15:37, Eric Bailey ***@***.***> wrote:
@lmartins gotcha, makes sense. Yeah picoapp assumes each component is just a function. So in this case, it would be pretty trivial to Promise.resolve(Component) and then call the returned function.
Do you feel like taking a stab at that? I suppose it would go here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Hey @lmartins! Hope you don’t mind, but I submitted an alternative PR ( #15 ) that effectively solves this feature request. My use-case called for resolving an arbitrary promise (not a dynamic import). So while it’s a bit more generic, it should do the trick for you: const FooterGallery = () => import('./components/media/FooterGallery')
// add component
app.add({
FooterGallery: FooterGallery.then(module => module.default)
})
// bind component to DOM
app.mount()
.then(() => console.log("FooterGallery has mounted")) // optionally run some code after `mount` resolves |
@bro-strummer Oh, I love what you did Tyler. Hoping that @estrattonbailey can provide some feedback on this too. Like you, I'm loving this tiny mighty library! |
Hey! Really appreciate both of you helping with this! Smart ideas, totally see where you're coming from. In fact, I've never actually gotten around to using dynamic imports on any project, so this is really helpful in terms of grokking what a solution will look like. Since you're both trying to solve very similar problems, let's just discuss both PRs here! Seems like both options do adequately solve the problem, though in slightly different ways. #13 handles modules on an individual level, whereas #15 resolves queue mounting modules as a batch. I see benefits with both, but I would think it's smart to err on the side of individual components being agnostic of how others are instantiated. Related to that, @bro-strummer, maybe you could clarify your use case for me? Is there a reason you can't resolve data first, then call mount? Or mount, but handle dependency resolution within the component? Also, I don't think this line will work with dynamic imports, since a dynamic component would need to be wrapped in a callable to avoid being resolved immediately on page load. This would work for your example though. I do also agree that I don't love the I think both of these are smart solutions, but they've also got me thinking – and I apologize for not thinking more about this way back when. How much mileage could we get out of this:
// component.js
export default (node, ctx) => {
// we don't need to return this promise here
import('./myAsyncComponent.js').then(module => {
// this component is basically just a normal picoapp component at this point
const instance = module.myExportedComponent(node, ctx) // or module.default
const unsubscribe = ctx.on('unmount', () => {
instance.unmount() // handle cleanup as needed
unsubscribe() // remove listener manually
})
})
} // index.js
import component from './component.js'
const app = picoapp({ component })
app.mount()
// after route change or whatever
app.unmount() Seems like a probably we could all spend a considerable amount of time solving and thinking about, so I apologize if I missed some minutiae within each of your solutions. Excited to hear your thoughts! |
@estrattonbailey I would lean towards @bro-strummer approach #15 , and would love to see the baked into the library. I'm currently patching my own version of the library just to have the functionality, and looking forward to just use the library as default again. |
Hey guys! Sorry to have left this cold—bit of a busy time for me. I've got a long weekend coming up and hope to put forth some consideration towards this issue then. |
@estrattonbailey could you please let us know if this is something you'd be willing to add to the library? |
Hey, I'm sorry y'all. Just moved from NYC to Chicago and haven't been online much. Hoping to get around to some open source over the holidays, starting next week. |
Hey all, sorry I'm just getting back to this. Been a busy couple of months! Definitely still interested in this idea, but I'm wondering if the "why" is worth the added complexity – however slight it is. I might need some more context on your use cases, but what I understand so far should already be possible with picoapp. Instead of adding code to core, do you think we could solve this with more docs and examples? Or another pattern that's opt-in, like a higher-order function? |
@estrattonbailey Hey Eric, thank you for following up on this. I think @bro-strummer did a tremendous job explaining the use case on his PR #15 so I'd be inclined to suggest that one instead. The use case is pretty simple but valuable, to be able to use the library while still making use of code-splitting features provided by Webpack. I've been using Tyler's implementation since November, would love to be able to get back to the "official" version. |
Hey! Totally agree, you've both done a great job of outlining what you'd like to see from picoapp, thank your for your attention to detail. However, what I'm saying is, using dynamic imports or loading data in conjunction with picoapp should already be possible without adding any code. Are you able to share a minimal example of what you're doing? A gist or even a codesandbox could work. I just want to make sure I could provide examples of your use cases. |
Hey @estrattonbailey @lmartins! Per my comment on #15, I actually no longer have a use case for this feature. I will be unpublishing my picoapp-async package from NPM. Anyone using it will eventually need to swap it out for Eric’s picoapp package. @lmartins I’ll wait a few weeks to unpublish it, so as not to pull the rug from under you. Also, if you need guidance in the swap, I am more than happy to help 😃. |
@tylerbrostrom Thanks so much for the heads up man! |
@lmartins here is one approach that works with Parcel bundler. /**
* Data structure listing modules that export a picoapp component.
* Rollup and Parcel will create a chunk per `import()`.
* These dynamic imports only load when they’re referenced (at least that’s the case when bundling with Parcel)
*/
const lazyComponents = {
video: import('./components/video-player.js'),
dialog: import('./components/dialog.js'),
}
/**
* Loads, registers (but does not mount) picoapp components exported from
* module chunks listed in `lazyComponents`
*/
export async function addLazyComponent(name, exportID = 'default') {
if (!Object.prototype.hasOwnProperty.call(lazyComponents, name)) {
throw new Error(`${name} is not a valid lazy component`)
}
app.add({
[name]: await lazyComponents[name].then((module) => module[exportID]),
})
}
/**
* Example usage
*/
document.getElementById('video').addEventListener('mouseover', async () => {
await addLazyComponent('video')
app.mount() // instantiate new components
}, { once: true }) |
Hey all, great library and great extensions here. I'm building a picoapp / webpack project, and am trying to get dynamic modules working. The goal is to have picoapp import the modules on |
Not sure if this is the ideal approach, but I ended up generalizing @estrattonbailey 's async component import. import ModuleFirst from './ModuleFirst'
const asyncComponentFactory = (handle) => {
return (node, ctx) => {
return import(
/* webpackMode: "lazy" */
`./${handle}.js`
).then(module => {
const instance = module.default(node, ctx)
const unsubscribe = ctx.on('unmount', () => {
instance.unmount()
unsubscribe()
});
})
}
}
// List all possible components
const components = {
ModuleFirst, // loaded immediately and bundled
ModuleSecond: asyncComponentFactory('ModuleSecond'), // loaded when resolved in the DOM and chunked
ModuleThird: asyncComponentFactory('ModuleThird'),
ModuleFourth: asyncComponentFactory('ModuleFourth'),
};
const app = picoapp(components); With this setup module chunks are only imported to the app when they are encountered by the picoapp module resolver. |
@ldrummond Yeah that’s a pretty slick solve for Webpack projects. AFAIK, neither Parcel nor Rollup have "magic code-splitting comments", so those not using Webpack will need to stick to my (recently edited) example or Eric’s example. |
Hi, thank you for this super useful library,
Have you played with adding the ability for the library to also deal with dynamic imports?
I'm using the feature provided by Webpack, which when passed to the picoapp instance it doesn't initiate the lazyloaded module.
Thank you!
The text was updated successfully, but these errors were encountered: