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

Fixing dynamic can-imports #9

Closed
matthewp opened this Issue Sep 26, 2016 · 11 comments

Comments

Projects
None yet
4 participants
@matthewp
Copy link
Contributor

matthewp commented Sep 26, 2016

One probably people often face is dynamically importing a module like so:

<can-import from="my/custom/element">
  <my-element/>
</can-import>

This will silently not do what you want, because the child hasn't been loaded.

I think we need to separate the loading part from the rendering part. Here's one idea:

<can-import from="my/custom/element" {^is-resolved}="*loaded" dynamic />

{{#if *loaded}}
  <my-element/>
{{/if}}
@matthewp

This comment has been minimized.

Copy link
Contributor Author

matthewp commented Mar 31, 2017

I wonder if lazy is a better name for the attribute, I feel people might understand it better.

@phillipskevin

This comment has been minimized.

Copy link
Contributor

phillipskevin commented Mar 31, 2017

After our discussion, I like @leoj3n's idea of making a separate tag for dynamic imports. I think this could be done in two parts...

  1. Make any <can-import ..s static if they do not contain content. That way either of these would be static imports:
<can-import from="my/custom/element" />
<can-import from="my/custom/element"></can-import>
  1. Make a separate <can-dynamic-import ...>...</can-dynamic-import> tag. This would do exactly the same thing <can-import ...>...</can-import> does now and would provide a warning if you don't have a closing tag (if we can do that).

This way, everywhere in our docs we show <can-import ... /> for static imports and <can-dynamic-import>...</can-dynamic-import> for dynamic imports.

@pYr0x

This comment has been minimized.

Copy link

pYr0x commented Apr 7, 2017

i agree with @phillipskevin and @leoj3n.
but i would go further.

for dynamic import

<can-dynamic-import ...>...</can-dynamic-import>

for static import

<can-static-import></can-static-import>
OR
<can-static-import/>

content within the <can-static-import> would throw a warning

@phillipskevin

This comment has been minimized.

Copy link
Contributor

phillipskevin commented Aug 28, 2017

After thinking about this some more, I think we should create a <can-dynamic-import> that can be used with or without a closing tag.

With:

<can-dynamic-import from="my/custom/element">
    {{#if isResolved}}
        ...
    {{/if}}
</can-async-import>

Without:

<can-dynamic-import from="my/custom/element" value:to="*customElementLoaded" />

{{#if *customElementLoaded.isResolved}}
    ...
{{/if}}

We should then deprecate using <can-import> for dynamic imports and update our docs / guides to use the new tag for dynamic imports.

@bmomberger-bitovi bmomberger-bitovi self-assigned this Sep 18, 2017

@phillipskevin

This comment has been minimized.

Copy link
Contributor

phillipskevin commented Sep 18, 2017

Here is what we're doing:

  • Create can-dynamic-import tag that works with closing tags
<can-dynamic-import from="my/custom/element">
// this is the promise
</can-dynamic-import>
  • Create can-dynamic-import tag that works with self-closing tag
<can-dynamic-import from="my/custom/element" isResolved:to="*myCustomElementLoaded" />
  • Convert can-import with closing tag and empty content to a static import
<can-import from="my/custom/element"></can-import>
@matthewp

This comment has been minimized.

Copy link
Contributor Author

matthewp commented Sep 18, 2017

For reference, the current self-closing logic is partially in https://github.com/canjs/can-stache/blob/master/src/intermediate_and_imports.js

done-autorender also uses this module so any breaking changes will need to be in a major version.

@pYr0x

This comment has been minimized.

Copy link

pYr0x commented Sep 19, 2017

The first time as a contributor I pronounce my veto. :)

I am not very happy about the current solution.

Instead of simplify the import we add more complexity to it.

E.g. Can-dynamic-import with self closing tag. That's confusing because you need to create a reference to a variable. In HTML all content inside of a tag becomes a child that inherits from its parent.
Here now we are trying to mimic JavaScript with references to promises that can resolve and used later on the context.

My preferred way to resolve this is:

  • use meaningful tags like can-static-import and can-dynamic-import
  • can static import should only used with self closing tags. Throw a warning if not
  • can dynamic import should only used with a closing tag
  • Throw warnings if the user try to use a approach that's not supported.

Don't be afraid of braking changes if it solves a problem in a better way

Please overthink this solution

Cc @matthewp @phillipskevin @bmomberger-bitovi @justinbmeyer

@matthewp

This comment has been minimized.

Copy link
Contributor Author

matthewp commented Sep 19, 2017

@pYr0x Sorry to hear this! :) . Let me try to summarize your issues so we can discuss it. I don't think we're really that far off in our thinking here.

  • Dislike using a variable to get a can-dynamic-import's promise.
  • Prefer can-static-import over can-import.
  • Prefer enforcing self-closing for static imports and not having self-closing for dynamic imports.

I hope I have fairly summarized your issues here. Will reply next.

@matthewp

This comment has been minimized.

Copy link
Contributor Author

matthewp commented Sep 19, 2017

Using variable on dynamic import

Just so it's clear, this still works and will likely be the most common way to use dynamic imports:

<can-dynamic-import from="path">
  {{#if isResolved}}
    ... can use whatever now
  {{/if}}
</can-dynamic-import>

And getting a variable from an import is not a new feature. Here's a forum post about how to do it in can 2.3. We need to be able to do this, as it's the main way to load a partial (with a static import though).

Since all element's with a view model can have their VM "pulled out" like this, it wouldn't make sense to restrict it only for can-dynamic-import.

Static import tag name

I don't have a strong opinion on this. I prefer the more concise name since static imports are what you use 90% of the time, so using a more verbose name would be a bummer, imo, but 🤷🏼‍♂️

Self-closing vs not self-closing

I'd argue that this is the biggest reason why this change is being made at all. Having the tag mean different things when it was self-closing vs. not was one of the most complained about parts of DoneJS.

Remember that in HTML there is no such thing as self-closing tags, except for a few built-in elements. Otherwise you always have to close them. But also remember that stache is not HTML, it's a templating language that produces HTML. Self-closing is a feature of stache, and people like it (not me!). I think having imports have special behavior when they are self-closing vs. not would be a mistake (one that this issue seeks to rectify).

@phillipskevin

This comment has been minimized.

Copy link
Contributor

phillipskevin commented Sep 19, 2017

Thanks @matthewp. To add on to this... the eventual goal is to make it so that the tags behave the same whether or not you use self-closing tags. So, these would behave the same (would import statically):

<can-import from="foo/bar"/>
<can-import from="foo/bar"></can-import

And these would behave the same:
<can-dynamic-import from="foo/bar"/>
<can-dynamic-import from="foo/bar">...</can-import>

This is not done now because this would be a breaking change (to make <can-import>{{#if isResolved}}...{{/if}}</can-import> a static import).

Perhaps, we should just add a <can-static-import> now so that we can accomplish this without the breaking change, but like @matthewp I also prefer the more concise name.

I think if we update our examples to use <can-import from="foo/bar" /> for all static imports and <can-dynamic-import>{{#if isResolved}}...{{/if}}</can-dynamic-import> for all dynamic imports that gets us 90% of the way there.

@bmomberger-bitovi

This comment has been minimized.

Copy link
Contributor

bmomberger-bitovi commented Sep 19, 2017

@pYr0x I had the same concerns you had. I'd rather have the can-dynamic-import tag use slots and let the default be handled by the resolver, so that markup writers didn't have to worry about promise values or using the refs scope. It wasn't the consensus option, though. From here on out you'll have to try making a PR if you want to see it go a different direction (and you can always tap me for help with working the parser).

There are legitimate reasons why we might want to use a dynamic import with a self closing tag, though. If the use of the import happens multiple places in the script, or there are multiple dynamic imports, this can prevent too much unnecessary nesting of tags.

Just to reiterate, nothing about current usage is changing other than we'll start recommending the dynamic import tag where warranted. Just that we're responding to common mistakes and making them not be mistakes anymore, to be friendlier to users. So:

<can-import/>  <!-- static -->
<can-import></can-import>  <!-- static -->
<can-import>   </can-import>  <!-- static -->
<can-import>
</can-import>  <!-- static -->
<can-import> foo </can-import>  <!-- dynamic -->
<can-import> <foo/> </can-import>  <!-- dynamic -->
<can-import> {{foo}} </can-import>  <!-- dynamic -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.