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

Handle dynamic import or importSync of missing module #547

Merged
merged 13 commits into from
Sep 25, 2020

Conversation

thoov
Copy link
Collaborator

@thoov thoov commented Sep 23, 2020

If a module is imported via import('foo') or importSync('foo') but foo cannot be resolved, instead create a dummy module that throws a runtime error stating that the module could not be found. The reasoning behind this change is that it is acceptable to have "guarded imports". An example is take ember-exam which does not explicitly depend on either ember-qunit or ember-mocha but has code like the following:

let emberTestFramework;

if (macroCondition(dependencySatisfies('ember-qunit', '*'))){
  emberTestFramework = importSync('ember-qunit');
} else if (macroCondition(dependencySatisfies('ember-mocha', '*'))) {
  emberTestFramework = importSync('ember-mocha');
}

During dev builds we do not strip out the conditionals that are falsy and thus when macros-babel-plugin runs it will try and resolve these imports (and cause a build-time error). This differs from 2 points of perspective: 1) classical system allow requiring a module that does not exist and you will not see any errors until that require is encountered at runtime (or if it is guarded it may never error). 2) The behavior of the macro system in production mode would hide the fact that this module was not found since it striped out the import branch which is different from how it would behave under dev mode.

Additional background on ember-exam's change: ember-cli/ember-exam#599

Fixes: #543
Fixes: #544

@thoov thoov requested a review from ef4 September 23, 2020 07:24
@thoov thoov changed the title [WIP] Handle dynamic import or importSync of missing module Handle dynamic import or importSync of missing module Sep 24, 2020
@thoov thoov marked this pull request as ready for review September 24, 2020 03:33
Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

Overall looking good, if you can incorporate these two suggestions and get the tests green.

packages/core/src/babel-plugin-adjust-imports.ts Outdated Show resolved Hide resolved
@thoov thoov force-pushed the adjust-imports-missing branch from aa764d9 to 0d74c8a Compare September 24, 2020 22:41
@thoov thoov requested a review from ef4 September 24, 2020 23:52
@thoov
Copy link
Collaborator Author

thoov commented Sep 25, 2020

@ef4 this should be ready to merge

@ef4 ef4 merged commit b56cf2a into embroider-build:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants