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

Disable the modules API polyfill on Ember 3.27+ #683

Merged
merged 5 commits into from
Mar 18, 2021

Conversation

pzuraq
Copy link

@pzuraq pzuraq commented Mar 16, 2021

Disables the modules API polyfill on the latest versions of Ember.

@pzuraq pzuraq force-pushed the disable-modules-api-polyfill-with-latest branch from 2c27d0f to 4d205a4 Compare March 16, 2021 22:44
Disables the modules API polyfill on the latest versions of Ember.
@pzuraq pzuraq force-pushed the disable-modules-api-polyfill-with-latest branch from bfafe95 to 45bdb10 Compare March 16, 2021 23:33
@rwjblue
Copy link
Member

rwjblue commented Mar 17, 2021

not ok 7 Chrome 89.0 - [48 ms] - tests/integration/components/test-inline-precompile: template literal proposal works
    ---
        actual: >
            null
        stack: >
            ReferenceError: _emberComponentTemplateOnly is not defined
                at Object._callee5$ (http://localhost:7357/assets/tests.js:508:19)
                at tryCatch (http://localhost:7357/assets/vendor.js:411:40)
                at Generator.invoke [as _invoke] (http://localhost:7357/assets/vendor.js:685:22)
                at Generator.prototype.<computed> [as next] (http://localhost:7357/assets/vendor.js:444:21)
                at asyncGeneratorStep (http://localhost:7357/assets/tests.js:348:105)
                at _next (http://localhost:7357/assets/tests.js:350:196)
                at http://localhost:7357/assets/tests.js:350:366
                at new Promise (<anonymous>)
                at Object.<anonymous> (http://localhost:7357/assets/tests.js:350:99)
                at Object.<anonymous> (http://localhost:7357/assets/tests.js:549:25)
        message: >
            Promise rejected during "template literal proposal works": _emberComponentTemplateOnly is not defined
        negative: >
            false
        browser log: |

@pzuraq
Copy link
Author

pzuraq commented Mar 17, 2021

Yeah, this PR is going to be dependent on

The first one is a fix that directly impacts the behavior of this addon, the second one is required in order to properly test this addon with the intended behavior

Disables the modules API polyfill on the latest versions of Ember.
@rwjblue rwjblue force-pushed the disable-modules-api-polyfill-with-latest branch from 45bdb10 to 26d9fdc Compare March 17, 2021 17:10
@rwjblue
Copy link
Member

rwjblue commented Mar 17, 2021

Rebased to pull in the babel-plugin-htmlbars-inline-precompile updates .

…:ember-cli/ember-cli-htmlbars into disable-modules-api-polyfill-with-latest
@pzuraq pzuraq force-pushed the disable-modules-api-polyfill-with-latest branch from 3871565 to fadc36c Compare March 17, 2021 21:20
@@ -24,7 +24,7 @@ function rethrowBuildError(error) {
}

class TemplateCompiler extends Filter {
constructor(inputTree, _options) {
constructor(inputTree, _options, requiresModuleApiPolyfill) {
Copy link
Member

Choose a reason for hiding this comment

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

Should probably default to true here. Otherwise, this is a breaking change.

state._colocationEnsureImport = (exportName, moduleName) => {
let addedImports = (allAddedImports[moduleName] = allAddedImports[moduleName] || {});

if (addedImports[exportName]) return addedImports[exportName];
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this suffer from the same problem that ember-cli/babel-plugin-htmlbars-inline-precompile#357 fixed?

Shouldn't this return a new identifier each time?

path.unshiftContainer('body', newImport);
}

return addedImports[exportName];
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this suffer from the same problem that ember-cli/babel-plugin-htmlbars-inline-precompile#357 fixed?

})
);

it('invokes AST plugins', async function () {
Copy link
Member

Choose a reason for hiding this comment

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

thanks for using async/await here, I just haven't had a chance to clean these other co.wrap's up yet

assert.strictEqual(outputString, expected);
assert.ok(outputString.includes('Huzzah!'));
});
let tree = new TemplateCompiler(input.path(), htmlbarsOptions, true);
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, adding true here is the thing I was referring to (as a breaking change). If you do require('ember-cli-htmlbars') the thing you get is the template compiler broccoli-plugin (for better or worse) which makes the constructor at least intimate API (but probably public).

@pzuraq pzuraq force-pushed the disable-modules-api-polyfill-with-latest branch from fadc36c to 3697751 Compare March 17, 2021 21:52
@@ -169,6 +170,9 @@ module.exports = {

let isProduction = process.env.EMBER_ENV === 'production';

let checker = new VersionChecker(this.parent).for('ember-source', 'npm');
Copy link
Contributor

@ef4 ef4 Mar 28, 2021

Choose a reason for hiding this comment

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

I suspect this is broken in second-level or deeper addons. They're looking for ember-source in their parent only, but should be looking at the top level project. Same kind of bug as emberjs/ember-cli-babel#396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants