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

[BUGFIX beta] Allow passing a string literal to {{helper}} and {{modifier}} #19487

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

chancancode
Copy link
Member

As per RFC #432

However, dynamic string values are currently disallowed per earlier framework team weekly meeting discussion to not make it harder for Embroider to heuristically analyze the dynamic usages.

Eventually we will want to do the same for components as well, and this AST transform would work there too.

// Bug: why do these fail?
// assert('[BUG] expecting a string literal for the `type` argument', isConstRef(typeRef));
// assert('[BUG] expecting a string literal for the `loc` argument', isConstRef(locRef));
// assert('[BUG] expecting a string literal for the `original` argument', isConstRef(originalRef));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pzuraq I don't think this has to block merging but it seems odd that we lost the const reference propagation here:

{{helper (-disallow-dynamic-resolution this.helperName type="helper" loc=" (some-file.hbs:1:2) " original="this.helperName") "helper args"}}
                                                            ~~~~~~~~     ~~~~~~~~~~~~~~~~~~~~~~~          ~~~~~~~~~~~~~~~~~

It seemed strange that isConstRef returned false on this. The logic in isConstRef checks for ref.tag === CONST_TAG but ref.tag is null here. Seems like a bug?

import { internalHelper } from './internal-helper';

export default internalHelper(({ positional }: CapturedArguments, owner: Owner | undefined) => {
// why is this allowed to be undefined in the first place?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pzuraq This seemed strange. I suspect this is leftover from when you thought you didn't have to propagate owner in strict mode and left the | undefined in the signature, even though in practice it shouldn't ever be undefined?


// BUG: this should work according to the RFC
// this.render(
// '[<div {{modifier "replace"}}>Nope</div>][<div {{modifier (modifier "replace") "wow"}}>Nope</div>]'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @pzuraq for upstream bug that we should fix before release (currently this gives an error message saying modifier keyword cannot be used as a modifier)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

() =>
this.render(
// BUG: this should work according to the RFC
// '<div {{modifier this.name}}>Nope</div>',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@chancancode chancancode force-pushed the dynamic-helper-modifier-resolution branch 4 times, most recently from fc0dba8 to 1b3960c Compare March 30, 2021 11:12
// has to be included anyway. In the future, perhaps we can avoid the latter by using
// `import(...)`?
BUILTIN_KEYWORD_HELPERS['-disallow-dynamic-resolution'] = disallowDynamicResolution;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @rwjblue though it probably doesn't have to block merging

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya we should fix, I agree. But it doesn't really matter for landing.

@chancancode chancancode force-pushed the dynamic-helper-modifier-resolution branch from 1b3960c to 80d052c Compare March 30, 2021 12:04
packages/@ember/-internals/glimmer/lib/helpers/-resolve.ts Outdated Show resolved Hide resolved
// has to be included anyway. In the future, perhaps we can avoid the latter by using
// `import(...)`?
BUILTIN_KEYWORD_HELPERS['-disallow-dynamic-resolution'] = disallowDynamicResolution;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya we should fix, I agree. But it doesn't really matter for landing.

@chancancode chancancode force-pushed the dynamic-helper-modifier-resolution branch from 80d052c to d483598 Compare March 30, 2021 12:41
…fier}}

As per [RFC #432](https://github.com/emberjs/rfcs/blob/master/text/0432-contextual-helpers.md#invoking-contextual-modifiers)

However, dynamic string values are currently disallowed per earlier
framework team weekly meeting discussion to not make it harder for
Embroider to heuristically analyze the dynamic usages.

Eventually we will want to do the same for components as well, and
this AST transform would work there too.
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

Successfully merging this pull request may close these issues.

2 participants