Skip to content
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

Open
lmartins opened this issue Aug 29, 2019 · 20 comments
Open

Compatibility with dynamic imports #7

lmartins opened this issue Aug 29, 2019 · 20 comments

Comments

@lmartins
Copy link

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!

@estrattonbailey
Copy link
Collaborator

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?

@lmartins
Copy link
Author

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:

import ComponentA from './components/ComponentA';
import ComponentB from './components/ComponentB';

const AppComponents = {
  ComponentA,
  ComponentB
}

const app = picoapp(AppComponents);

Each component in this format returns the component instance, whereas the Webpack dynamic import returns a promise instead.

The components definition would be pretty similar:

const FooterGallery = () => import('./components/media/FooterGallery');

But what this FooterGallery holds is a promise as detailed here:
https://webpack.js.org/guides/code-splitting/#dynamic-imports

So I picoapp would need to determine the component initialisation based on whether the returned value would be a promise or a standard constructor?

@estrattonbailey
Copy link
Collaborator

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

@lmartins
Copy link
Author

lmartins commented Sep 11, 2019 via email

@tylerbrostrom
Copy link

tylerbrostrom commented Nov 4, 2019

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

@lmartins
Copy link
Author

lmartins commented Nov 5, 2019

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

@estrattonbailey
Copy link
Collaborator

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 module.default resolution, though I've had to do similar things in the past and would agree that if #13 is the best solve, it makes sense to go that route.

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:

  1. Keep "mount" synchronous
  2. Keep component agnostic of each other and individual dependencies
  3. Use events to synchronize and handle cleanup
    • so might want to add mount and unmount events
// 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!

@lmartins
Copy link
Author

lmartins commented Nov 27, 2019

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

@tylerbrostrom
Copy link

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.

@lmartins
Copy link
Author

@estrattonbailey could you please let us know if this is something you'd be willing to add to the library?

@estrattonbailey
Copy link
Collaborator

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.

@estrattonbailey
Copy link
Collaborator

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?

@lmartins
Copy link
Author

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

@estrattonbailey
Copy link
Collaborator

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.

@tylerbrostrom
Copy link

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

@lmartins
Copy link
Author

@tylerbrostrom Thanks so much for the heads up man!
I do use your version on a few projects. Would you mind share a little snippet of how you approach it so I can drop the dependency?

@tylerbrostrom
Copy link

tylerbrostrom commented May 27, 2020

@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 })

@ldrummond
Copy link

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 mount when as it resolves them from the html, so it only loads the necessary modules. Did any of you reach a consensus on how best to achieve that?

@ldrummond
Copy link

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.

@tylerbrostrom
Copy link

but I ended up generalizing @estrattonbailey 's async component import.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants