Accudio / async-alpine

Async Alpine brings advanced code splitting and lazy-loading to Alpine.js components!
https://async-alpine.dev
Apache License 2.0
152 stars 12 forks source link

Doesn't support dynamic content #11

Closed Accudio closed 2 years ago

Accudio commented 2 years ago

If content is loaded with ax-load present, it won't be handled as an asynchronous component.

This may be tricky to implement, we'd have to use a mutation observer and ensure the DOM is changed before Alpine's mutation observer can be triggered.

This would prevent Async Alpine from working with client-side routers and similar, which is a fairly major area that Alpine works well in.

Needs more research.

Accudio commented 2 years ago

Need more information and testing about whether we can run watch for mutations with MutationObserver and modify the DOM before Alpine's MO runs.

Potential questions/challenges:

Accudio commented 2 years ago

Done some experimenting and it's not promising. As far as I can tell there is no way to manage this without somehow processing before MutationObservers or a modification to Alpine internals.

Alpine uses the static NodeList to know what has changed, which means even though we could declare a MO earlier the changes we make aren't picked up.

I think the only neat resolution to this—unless there's a way of manipulating MO to pass on live elements—is a change to Alpine core to check the real element for attributes rather than the MO NodeList.

The only other alternative would be implementing a meta syntax on top of Alpine—hardly ideal.

iniznet commented 2 years ago

I'm not sure if this was the right way, but I created an experimental lazy load for AlpineJS for curiosity in a messy way.

As you said, I always hit a wall too when it comes to dynamic/SPA content due to Alpine's observer always getting them first before I'm able to intercept them so I instead decided to register components during initialization and destroy them after the download is done, then reinit them again with the downloaded module.

Based on this information, https://github.com/alpinejs/alpine/blob/3dbdcfb58033c9b8687cb4c5260808e1297dd428/packages/alpinejs/src/datas.js#L4-L6

I think this is also my answer to #7

document.addEventListener('alpine:init', () => {
  ...

  /** Components */
  dynamicData(Alpine)
    .data('form', () => import ('./components/form')) // I like this way better than expose my asset path in html
    .init();

  ...
});

Inside the dynamicData()

export default function dynamicData(Alpine) {
  return {
    data(name, component) {
      config.components[name] = component;

      // early register to prevent Alpine's observer throws an error
      Alpine.data(name, () => ({
        loading: true,

        init() {
          this.$el.setAttribute('x-ignore', ''); // disable component & ignores other directives right away
        }
      }));

      return this;
    },
    init() {
      // start watching with MutationObserver
      start(Alpine);
    }
  }
}

In my own observer, it's only watching new elements as Alpine already handle others and initialize existing ones that have ax-load attribute.

download() & activate()

async function download(Alpine, component, name) {
  const module = await component();

  component = module[component] || module.default || Object.values(module)[0] || false;

  Alpine.data(name, component); // override existing data that registered by data()
}

async function activate(Alpine, element) {
  await element.removeAttribute('ax-load');
  await element.removeAttribute('x-ignore');

  await (async () => walk(element, el => cleanupAttributes(el)))(); // destroy the component
  Alpine.initTree(element); // then init the component again as we have downloaded module take the place
}

Implementation

<div class="mt-10">
  <form
    x-data="form('do_register')"
    @submit.prevent="send"
    method="POST"
    ax-load
  >
  <div class="" x-show="loading"><h1 class="text-xl">LOADING!!!!</h1></div>
</div>

the loading property actually still works and Alpine doesn't throw an error either, it automatically hides the element after the data is overridden.

iniznet commented 2 years ago

An alternative way is by using Alpine.mutateDom though I'm not really sure about this option, however, not x-data only we might also need to rename other directives as well as they will link to the parent component or just add x-ignore attribute instead of renaming. https://github.com/alpinejs/alpine/blob/3dbdcfb58033c9b8687cb4c5260808e1297dd428/packages/alpinejs/src/mutation.js#L83-L93

Accudio commented 2 years ago

@iniznet Your approach is really interesting and looks like it could be a great solution. I'll need to give this a try for Async Alpine, unless you want to implement and PR it yourself? I'd be open to that.

I attempted something similar when I was first prototyping Async Alpine, but ran into the issue of undefined properties with the dummy component. I didn't think of using x-ignore at the time, I really like how that cleans things up. I've been preferring a rename pattern to allow for independent loading of nested components but if I have to sacrifice that to have a nice dynamic loading API I will.

I couldn't find a way of jumping on Alpine.mutateDom unfortunately, that didn't seem like a realistic solution.

iniznet commented 2 years ago

I will pass this time, leave it to your way. Unfortunately, I don't have enough experience to be able to give the best practice I ever think of.

At first, I'd tried to only use x-ignore when there are page changes with observer but it's only able to intercept the first imported component, then Alpine throw a component is not defined error for the rest lazy components. I found it weird that after the module is imported, it's able to bring the lazy component to start working alongside the error still present and that is where the solution of doing early register data with Alpine.data() come to my mind.

Yeah, I don't find a way either on Alpine.mutateDom in my experiment. My second thought was coming from this comment https://github.com/alpinejs/alpine/issues/359#issuecomment-973688464

Accudio commented 2 years ago

Thanks for your suggestion, I really appreciate it. When I get time I'll take a look at re-engineering to use a dummy component and x-ignore.

Accudio commented 2 years ago

I've implemented a 0.4.0 version in the 0.4-rebuild branch using this method and it works really well!

Overall I'm really happy with this rebuild and with how this improves the project. I need to test it further with some larger projects, and would like to test it to see how it works with other build systems—particularly Vite. Merging and the 0.4 release will come after this testing and updating documentation.

Thank you @iniznet for your fantastic contribution to this issue, you've helped sort every issue I was having in one fell swoop!

iniznet commented 2 years ago

Glad to be of help! I'm looking forward to the finalized build. However, I'm not sure what you mean by Breaking: In order to support dynamic content, the names of inline components need to declared in advance with .inline('name')

Does that mean we need to use .inline('name') for every component to support dynamic content?

Accudio commented 2 years ago

Yes, as far as I know. Because the mutation observer runs first, the names of components need to be registered before the mutation happens. That's fine when components are registered in JS, but for components registered in HTML they need to be declared beforehand in JS.

That's what I've got for the current 0.4 beta, but when I get time next week I'd like to dig into Alpine first to confirm.

iniznet commented 2 years ago

I see, that fair enough for now.

Hey, I got Alpine Expression Error: Illegal invocation for putting the x-show="open" when doing a test with a dynamic data.

<div
  ax-load
  x-data="accordion"
>
  <div class="content" x-show="open">
    ... some content here ...
  </div>

  <button
    type="button"
    class="btn btn-primary"
    @click="open = !open"
  >Toggle</button>
</div>

accordion.js

export default function accordion() {
  return {
    open: false,

    toggle() {
      this.open = !this.open;
    }
  };
}

app.js

document.addEventListener('alpine:init', () => {
   ...
});

AsyncAlpine.init(Alpine);
AsyncAlpine.data('accordion', () => import('./components/accordion'));
AsyncAlpine.start();

Alpine.plugin(Persist);

Alpine.start();

Is there some mistake that I am unaware of here? I'm using async-alpine.esm.js

Accudio commented 2 years ago

@iniznet Could you share more of your initial example as a working example, perhaps in a repo or CodePen?

My initial test cases were fairly simplistic and it worked fine with them, but as you say with your example there are issues with x-show getting an is not defined error*. This is despite the component adding x-ignore immediately within the init() function.

It would be helpful to see your implementation and see if it suffers from the same issue.

This unfortunately would be a critical issue with the implementation as-is, but at this point I'd be happy to require x-ignore specified on components within markup to make it work. Hence it wouldn't be a blocking error.

* In your case you got an "Illegal invocation" error, not "is not defined". This is because the browser has an in-built open function that Alpine is calling since a locally scoped variable isn't defined.

Accudio commented 2 years ago

Further to the above I decided to look in more detail about dynamically setting x-ignore within a component and with a reduced test case it seems that it doesn't prevent Alpine from processing directives.

Unless I'm mistaken or missing something that @iniznet added I don't think this strategy is possible.

In the linked demo see how even without Async Alpine setting x-ignore within init still throws "is not defined" errors in the console.

iniznet commented 2 years ago

It seems so, my test also produces same result. The component already processing directives before init() is able to set the x-ignore and only finally works when the dummy data is overridden with real data.

My thought about this was to use a custom attribute for x-data like ax-data, so we put the x-ignore and rename the ax-data to x-data on every component during initialization, I'm not sure this method is preferable as we will lose loading state unless there's a way to make it still work, and here the test.

iniznet commented 2 years ago

Here the PR on my own fork to see what I changed,

https://github.com/iniznet/async-alpine/pull/1 codesandbox

Accudio commented 2 years ago

@iniznet Thank you for the fork, I don't think that strategy works though.

When I was initially testing using x-ignore instead of attribute renaming I used a similar strategy to dynamically add it. One key feature I want to retain is allowing ax-load components to be nested within x-data components, and that setup wouldn't work with those mutations thanks to Alpine getting a static MutationEntry not live nodes.

See how this CodeSandbox has the same "is not defined" error when added dynamically in a Mutation.

With the init() approach not working, I can't think of a way in which we can dynamically add content without x-ignore already being present. In that circumstance I think the simplest option would be to keep syntax as close to Alpine as possible and require developers adding x-ignore to any Async components.

iniznet commented 2 years ago

That's right, with how current Alpine works we don't really have any option other than to use x-ignore together with x-data when registering a component since it was the top priority in lifecycle, however, do we have some way though to technically have a loading state during the module download process?

I usually put a loading template or some process during loading state, maybe we can have a custom attribute like ax-loading and remove it when the component module is loaded.

Accudio commented 2 years ago

If you want to be able to have a state for before the component has been initialised I think you could already do with Alpine, by having a default attribute and removing it as soon as Alpine initialises the component.

Is there a reason why this pattern wouldn't work for you?

<div
  ax-load
  x-data="accordion"
  data-loading
  :data-loading="false"
>
  ...
</div>

Or do you mean for specifically when the component is downloading? If it's possible with Alpine I'd prefer to keep it that way but if it can't be I'd be happy to consider it.

Accudio commented 2 years ago

As for the architecture of 0.4 of Async Alpine, I think I'm going to go for continuing to use x-ignore and telling developers to add it to all components.

In actual fact we'll add it to any components that don't have it on load, so technically it would be optional for non-dynamic content but strictly necessary for dynamic content.

I'm considering just saying that it's required in the documentation though for simplicity.

iniznet commented 2 years ago

If you want to be able to have a state for before the component has been initialised I think you could already do with Alpine, by having a default attribute and removing it as soon as Alpine initialises the component.

Is there a reason why this pattern wouldn't work for you?

<div
  ax-load
  x-data="accordion"
  data-loading
  :data-loading="false"
>
  ...
</div>

Or do you mean for specifically when the component is downloading? If it's possible with Alpine I'd prefer to keep it that way but if it can't be I'd be happy to consider it.

Yeah, I usually do that. After thinking a while, the component init() should be able to do the job in case I need to process something before display the component.

There's PR called "Enhance directive to accept order option" in Alpine repo that might be interesting to be discussed once it merged, hopefully.

Accudio commented 2 years ago

Oh that is very interesting and I believe would allow us to add a high-priority directive that can add x-ignore before it's processed, thank you for flagging it!

When I get a chance I'm going to tweak the 0.4-rebuild to rely on a constant x-ignore and I can start testing. Once I've confirmed it works I'll move discussion to a new issue so keep titles meaningful.

If the Order Directive PR is merged we can revisit it and treat it as a non-breaking enhancement to remove the requirement for x-ignore.

Accudio commented 2 years ago

Moving the discussion over to to #20 since this has now been implemented in a beta version.