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

Allow macros config to not have a runtime implementation #1804

Closed
simonihmig opened this issue Feb 15, 2024 · 2 comments
Closed

Allow macros config to not have a runtime implementation #1804

simonihmig opened this issue Feb 15, 2024 · 2 comments

Comments

@simonihmig
Copy link
Collaborator

The following pseudo-code show the problem we are running into:

  if (
    macroCondition(
      getConfig<{ isEnabled: boolean }>('mocking-addon').isEnabled,
    )
  ) {
  let mockingStuff = [
    importSync('package-a/mock-stuff').default,
    importSync('package-b/mock-stuff').default,
    importSync('package-c/mock-stuff').default,
    // ...
  ];
  setupMock(mockingStuff);
}

This is supposed to not bundle the mocking code when mocking is not enabled, especially for production builds. I think this would not cause any trouble if we had all those package-* as v2 addons.

But some are v1, and as their modules would get eagerly bundled into the app, we have some broccoli-funnel logic in place that removes their /mocking-stuff/** modules according to the same logic that sets the isEnabled flag on mocking-addon. But this causes a problem now, as in a non-production build (with isEnabled being turned to false) eai would still see those importSync calls, add all those package-*/mock-stuff modules as dependencies of the AMD shim, but some of them don't exist (those from v1 addons), causing loader.js runtime errors.

This is a bit similar to embroider-build/ember-auto-import#582, which we "solved" by the optimization done in #1468, that had the pleasant side-effect of removing those importSyncs due to eager branch elimination. And I wonder if we could leverage the same approach here, by having some way to threat get*Config as a build-time only macro without a runtime implementation.

AFAICT, the API where you could actually change config at runtime seems to be in this obscured place, and it also seems to be only used by us (Embroider itself) for that fastboot detection, is that correct?

If so, could we think of ways to either opt-in into a static way of handling the config (thereby enabling eager branch elimination), or even do that by default and only opt-in into a runtime backed implementation when we have detected FastBoot?

@simonihmig
Copy link
Collaborator Author

simonihmig commented Feb 22, 2024

Preparing myself to work on this, I was thinking we need to make the evaluator just smart enough to cover this:

expression hasRuntimeImplementation
getConfig('foo') false
getConfig('foo').whatsoever false
getOwnConfig() false
getOwnConfig().whatsoever false
getGlobalConfig() false
getGlobalConfig().whatsoever false
getGlobalConfig().fastboot false true
getGlobalConfig().fastboot.isRunning true

Does that sound about right @ef4?

We agreed to make this backwards compatible to not force a major version for macros. But to have a way out of this special handling for whenever we do a new major version, should we introduce a dedicated isFastboot() or (isFastbootRunning() ?) macro for this special use case, and deprecate getGlobalConfig().fastboot.isRunning?

FWIW, a code search on emberobserver revealed that

  • there is just one (OSS) addon checking for fastboot.isRunning: ember-head
  • no addon is making use of _embroider_macros_runtime_config

@MrChocolatine
Copy link

#1815 has been merged but this issue did not auto-close despite the link "Fixes <issue_id>".

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

No branches or pull requests

2 participants