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

Deprecations will fire spuriously under strict ES module builds #9310

Closed
ef4 opened this issue Mar 29, 2024 · 4 comments
Closed

Deprecations will fire spuriously under strict ES module builds #9310

ef4 opened this issue Mar 29, 2024 · 4 comments

Comments

@ef4
Copy link
Contributor

ef4 commented Mar 29, 2024

Using ember-data 5.3.0 and given either:

ember-data emits deprecation warning for features that the app isn't actually using.

DEPRECATION: You are relying on ember-data auto-magically installing the BooleanTransform. Use `export { BooleanTransform as default } from 'ember-data/serializer/transform';` in app/transforms/boolean.js instead [deprecation id: ember-data:deprecate-legacy-imports] This will be removed in ember-data 6.0.
DEPRECATION: You are relying on ember-data auto-magically installing the DateTransform. Use `export { DateTransform as default } from 'ember-data/serializer/transform';` in app/transforms/date.js instead [deprecation id: ember-data:deprecate-legacy-imports] This will be removed in ember-data 6.0.
DEPRECATION: You are relying on ember-data auto-magically installing the NumberTransform. Use `export { NumberTransform as default } from 'ember-data/serializer/transform';` in app/transforms/number.js instead [deprecation id: ember-data:deprecate-legacy-imports] This will be removed in ember-data 6.0.
DEPRECATION: You are relying on ember-data auto-magically installing the StringTransform. Use `export { StringTransform as default } from 'ember-data/serializer/transform';` in app/transforms/string.js instead [deprecation id: ember-data:deprecate-legacy-imports] This will be removed in ember-data 6.0.

Historically Ember has allowed "modules" to remain un-evaluated until they are synchronously required. That is not possible under true ES modules. So deprecations in module scope like here:

deprecate(
"You are relying on ember-data auto-magically installing the StringTransform. Use `export { StringTransform as default } from '@ember-data/serializer/transform';` in app/transforms/string.js instead",

will fire before the app boots regardless of whether anybody is actually using that feature.

(One might also ask whether we can stop putting these modules in the build at all when they're not in use, and that's a good goal for the next-gen ember-data API, but these particular modules exist for backward compatibility and probably need to continue getting the backward-compatible behavior that pushes them into the build to make them synchronously available at runtime just in case they're used.)

@runspired
Copy link
Contributor

runspired commented Mar 29, 2024

Historically Ember has allowed "modules" to remain un-evaluated until they are synchronously required. That is not possible under true ES modules.

This sounds like a bundler error then, not a spec error so I wouldn't phrase it as "under true ES modules". By spec, a module should not even be loaded until a module which requires it is evaluated, and so a spec compliant bundler would in fact not eagerly evaluate the module unless accessing a specific entry point ultimately led to accessing the file.

As regards EmberData, overall I'm incline to believe we shouldn't do anything different here. We ship native ESM modules and we aren't what is responsible for causing those to be pushed into the build automatically. Transforms get pushed in because our tooling historically assumes that all code in the app directory needs to be pushed in, and because transforms are usually used via ember's resolver and so need to be there synchronously (they don't have to be used that way fwiw).

With this said, if you get this deprecation then almost by definition you should get this deprecation, because even if you don't think you are using these transforms you are and should go ahead and make their inclusion public, else drop ember-data as a dep and consume the individual packages so that they aren't guaranteed to be available to the resolver.

If any files print this that aren't the transforms, I'll reopen this to pursue a pragmatic fix.

@ef4
Copy link
Contributor Author

ef4 commented Mar 29, 2024

By spec, a module should not even be loaded until a module which requires it is evaluated

Yes, but "requires" is an imprecise word here. What you mean is "imports it", because importing a module is the only legal way to depend on it. And ember-data doesn't ever import the transforms, which is the only reason we're having this discussion.

ember-data is going to do getOwner(this).lookup('transform:boolean') an expect to synchronously get back the contents of the ES module. That absolutely means that the module must have already been evaluated earlier, because there's no way to synchronously evaluate an ES module that wasn't already evaluated.

@runspired
Copy link
Contributor

ember-data is going to do getOwner(this).lookup('transform:boolean') an expect to synchronously get back the contents of the ES module

nit: a specific serializer we currently provide does do this though we consider it to be app code if an app makes use of it. Most of the time, it will be app code that does the resolver lookup. EmberData itself makes no use of Ember's resolver outside of adapterFor and serializerFor, both of which are also optional hooks and can be easily implemented to not use the resolver.

@runspired
Copy link
Contributor

My general takeaway is the transform deprecation printing in this situation is desirable and not spurious, it just alerts the user to a requirement they may not have been paying attention to. If they truly don't want the code, then it might be time to consider bringing less of EmberData :D

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

No branches or pull requests

2 participants