marko-js / marko

A declarative, HTML-based language that makes building web apps fun
https://markojs.com/
MIT License
13.25k stars 643 forks source link

False deprecation warning on namespaced non-marko components… #1367

Closed hedefalk closed 2 years ago

hedefalk commented 5 years ago

Marko Version: 4.18.4

Details

I use some facebook markup that I let facebook parse from DOM:

      <fb:login-button 
        auto-logout-link="false"
        button-type="continue_with" 
        size="large"
        scope="public_profile,email" 
        use-continue-as="true">
      </fb:login-button>

In my onMount I then let the facebook API do it's thing with that and it works fine:

FB.XFBML.parse(component.getEl(…)

Expected Behavior

No warnings

Actual Behavior

I get deprecation warnings for nested syntax:

MIGRATION
The "<my-tag:nested>" tagName syntax is deprecated. Please use the "<@nested>" tagName syntax instead. See: https://github.com/marko-js/marko/wiki/Deprecation:-legacy-nested-tag
  at src/web/marko/components/fb-login-modal.marko:11:6

I guess this is a corner case when I stick non-marko custom components in there, but maybe it would be possible to suppress these warnings for non-marko components. dunno…?

mlrawlings commented 5 years ago

Thanks for reporting!

Yes, this migration should be modified so that it only does this if there is a parent tag that matches the part before :.

For example, this would still go through the migration path:

<fb>
    <fb:login-button 
        auto-logout-link="false"
        button-type="continue_with" 
        size="large"
        scope="public_profile,email" 
        use-continue-as="true">
    </fb:login-button>
</fb>

But <fb:login-button> alone would not.

mlrawlings commented 5 years ago

If anyone is interested in getting their feet wet contributing to Marko, this would be a good first issue to tackle. If you want to work on this, comment here and feel free to ask questions!

Implementation

The relevant code is here: https://github.com/marko-js/marko/blob/535daf94512e6e01a1f3d79b77b55d6ff35d2cbf/src/core-tags/migrate/all-tags/legacy-nested-tag.js#L9-L11

This if should add a check so that is also returns early if there is no ancestor that matches the nested tag prefix (fb in this example).

You can walk the ancestors by asking for the current el's parentNode

The logic would be kinda similar to this: https://github.com/marko-js/marko/blob/101c5c6bb856a0171fef7f41cc71258093d1947a/src/core-tags/migrate/util/findBoundParent.js#L1-L7

Except instead of searching for a parent with w-bind you're searching for a parent with a specific name.

Testing

It can be tested by adding a top-level tag that shouldn't be migrated here: https://github.com/marko-js/marko/blob/535daf94512e6e01a1f3d79b77b55d6ff35d2cbf/test/migrate/fixtures/legacy-nested-tag/template.marko#L1-L9

Something like

<foo:bar>
  This shouldn't be migrated
</foo:bar>

And updating the expected snapshot to assert that is is in fact not migrated: https://github.com/marko-js/marko/blob/535daf94512e6e01a1f3d79b77b55d6ff35d2cbf/test/migrate/fixtures/legacy-nested-tag/snapshot-expected.marko#L1-L6

It should still be

<foo:bar>
  This shouldn't be migrated
</foo:bar>
tigt commented 4 years ago

I’m interested in tackling this, as I had the same problem when using Marko to generate an OpenSearch description file.

(Also making a note here so I remember: this would also close #1297)

DylanPiercey commented 2 years ago

The current recommendation is to either: