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

[FEATURE default helper manager] glimmer vm upgrade + default helper manager / plain functions as helpers #19973

Conversation

NullVoxPopuli
Copy link
Contributor

No description provided.

@NullVoxPopuli NullVoxPopuli changed the title [Feature default helper manager] glimmer vm upgrade + default helper manager / plain functions as helpers [FEATURE default helper manager] glimmer vm upgrade + default helper manager / plain functions as helpers Feb 15, 2022
@NullVoxPopuli NullVoxPopuli force-pushed the update-glimmer-vm-019a8a9cps branch from 1d4c2df to 536a13f Compare February 15, 2022 17:18
@NullVoxPopuli NullVoxPopuli force-pushed the update-glimmer-vm-019a8a9cps branch from 536a13f to ab3e9bc Compare February 15, 2022 17:31
@Windvis
Copy link
Contributor

Windvis commented Mar 13, 2022

I created a PR (glimmerjs/glimmer-vm#1384) to add the needed feature flag on the Glimmer side: glimmerjs/glimmer-vm#1384

When I linked it into my local ember-source repo I did notice some tests were failing on the Ember side, so it probably requires more changes than simply bumping the version and enabling the feature. I might be wrong though, it was getting late 😅.

@Windvis Windvis mentioned this pull request Mar 13, 2022
@chancancode
Copy link
Member

I released the flag @Windvis added as 0.84.1 so we can proceed with this (w.r.t. the remaining steps in glimmerjs/glimmer-vm#1383). Note that in Ember, we should default the feature to be off unless specifically turned on by the ember feature flag (the ember feature flag's value should in turns be used to set the glimmer flag @Windvis added)

@chancancode
Copy link
Member

Specifically:

  1. Add flag to https://github.com/emberjs/ember.js/blob/master/packages/%40ember/canary-features/index.ts#L14-L18
  2. In
    setGlobalContext({
    scheduleRevalidate() {
    _backburner.ensureInstance();
    },
    toBool,
    toIterator,
    getProp: _getProp,
    setProp: _setProp,
    getPath: get,
    setPath: set,
    scheduleDestroy(destroyable, destructor) {
    schedule('actions', null, destructor, destroyable);
    },
    scheduleDestroyed(finalizeDestructor) {
    schedule('destroy', null, finalizeDestructor);
    },
    warnIfStyleNotTrusted(value: unknown) {
    warn(
    constructStyleDeprecationMessage(String(value)),
    (() => {
    if (value === null || value === undefined || isHTMLSafe(value)) {
    return true;
    }
    return false;
    })(),
    { id: 'ember-htmlbars.style-xss-warning' }
    );
    },
    assert(test: unknown, msg: string, options?: { id: string }) {
    if (DEBUG) {
    let id = options?.id;
    let override = VM_ASSERTION_OVERRIDES.filter((o) => o.id === id)[0];
    assert(override?.message ?? msg, test);
    }
    },
    deprecate(msg: string, test: unknown, options: { id: string }) {
    if (DEBUG) {
    let { id } = options;
    if (id === 'argument-less-helper-paren-less-invocation') {
    throw new Error(
    `A resolved helper cannot be passed as a named argument as the syntax is ` +
    `ambiguously a pass-by-reference or invocation. Use the ` +
    `\`{{helper 'foo-helper}}\` helper to pass by reference or explicitly ` +
    `invoke the helper with parens: \`{{(fooHelper)}}\`.`
    );
    }
    let override = VM_DEPRECATION_OVERRIDES.filter((o) => o.id === id)[0];
    if (!override) throw new Error(`deprecation override for ${id} not found`);
    // allow deprecations to be disabled in the VM_DEPRECATION_OVERRIDES array below
    if (!override.disabled) {
    deprecate(override.message ?? msg, Boolean(test), override);
    }
    }
    },
    });
    , set the value based on the flag
  3. Add/update the relevant tests taking into account the value of the flag

@ef4
Copy link
Contributor

ef4 commented Apr 14, 2022

This was superseded by #20052.

@ef4 ef4 closed this Apr 14, 2022
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.

4 participants