remove some usage of runtime require #2297
Conversation
`require` a pre-ES-module feature that continues to work in Ember but is fragile and unreliable under ES-module-first build tooling like Embroider. This PR replaces two uses with embroider macros instead, which are designed to allow dynamism without sacrificing static analysis.
|
|
||
| /** | ||
| @hide | ||
| */ | ||
| export const hasEmberData = _hasEmberData(); | ||
| export const hasEmberData = dependencySatisfies('ember-data', '*'); |
There was a problem hiding this comment.
Thanks, I never liked the way it checked before.
| @@ -1,4 +1,4 @@ | |||
| import require from 'require'; | |||
| import { importSync, dependencySatisfies, isTesting } from '@embroider/macros'; | |||
There was a problem hiding this comment.
Thanks. This entire file should be done away with in the next version
package.json
Outdated
| "peerDependencies": { | ||
| "@ember/test-helpers": "*", | ||
| "ember-qunit": "*" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "@ember/test-helpers": { | ||
| "optional": true | ||
| }, | ||
| "ember-qunit": { | ||
| "optional": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
Could you elaborate on why these are here. They are listed under devDependancies. Why are these two peer and optional and not the rest? Trying to understand if I should be doing something like this in other addons.
There was a problem hiding this comment.
dependencySatisfies follows the real NPM package resolving rules.
The rule is that a package looks in its own node_modules folder and then proceeds to look up the filesystem for higher node_modules folders.
An app will often accidentally allow ember-cli-mirage to resolve the app's copy of ember-qunit:
/the-app
└─ node_modules
└── ember-qunit
└── ember-cli-mirage
But this is not guaranteed. For example, if the app lives in a monorepo with many projects, some dependencies may be shared at the top level and others may not:
/the-monorepo
└─ node_modules
│ └── ember-cli-mirage
└── app1
│ └── node_modules
| └── ember-qunit
In this case, app1 can see both addons, but ember-cli-mirage cannot resolve ember-qunit, so dependencySatisfies would return false.
NPM's solution to this problem is peerDependencies. They declare that you must see the same copy as your parent package. With a peerDependency declaration, NPM would ensure that ember-cli-mirage and ember-qunit are always located so that ember-cli-mirage can see the correct copy of ember-qunit.
Usually peerDependencies also appear in devDependencies. That's because they're simultaneously dependencies of the dummy app and peerDependencies of the addon.
I marked these as optional peer dependencies because the existing code also treats them as optional. By declaring these optional peer deps, we're telling NPM that if the app has these packages, we need to be able to see them too, but if it doesn't have them that's OK.
There was a problem hiding this comment.
I knew most of what you said, what I missed was the dependencySatisfies is why you did this. @ember/test-helpers and ember-data appears in dependencySatisfies.
I had wanted to make ember-qunit a peerDependancy much like ember-qunit made qunit a peerDependancy. In your example of the mono-repo, should not every devDependancy be listed as a peer because we cant predict how the end consumer will structure their repo?
Also should ember-data be listed as a peerDependancy since it is directly referred to in a dependencySatisfies?
There was a problem hiding this comment.
You're right, it would be correct to add ember-data to peerDeps too. Done.
|
Yes, I downgraded back to the older embroider/macros for Node 10 support. |
|
@ef4 Thank you! |
requirea pre-ES-module feature that continues to work in Ember but is fragile and unreliable under ES-module-first build tooling like Embroider.This PR replaces two uses with embroider macros instead, which are designed to allow dynamism without sacrificing static analysis.