embroider-build / ember-auto-import

Zero config import from npm packages
Other
360 stars 109 forks source link

Provide modules to host app or control host app imports #170

Closed buschtoens closed 5 years ago

buschtoens commented 5 years ago

I think we already had an issue about this, but I can't seem to find it.

I am in the process of migrating ember-apollo-client to TypeScript and upgrading it to the latest and greatest of all its dependencies and seem to have hit a brick wall. 🚧

https://github.com/bgentry/ember-apollo-client/pull/175#issuecomment-448005937:

start-pretender.js imports graphql and graphql-tools. Since this file is part of the dummy application tree, it is managed by a different instance of ember-auto-import. Importing graphql inside of ember-apollo-client creates a standalone copy of it, that uses the exact same source code, but is not the same identity.

Basically what happens is that the same wrapping module factory function gets executed twice.

This then causes the following error:

Error: Cannot use GraphQLSchema "[object GraphQLSchema]" from another module or realm.

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

Duplicate "graphql" modules cannot be used at the same time since different
versions may have different capabilities and behavior. The data from one
version used in the function from another could produce confusing and
spurious results.

I assume that this used to work before, because we were using an outdated version of graphql, that probably did not perform this check, but I am not 💯 on that.

We need to be able to control where the host app gets its graphql import from. We either do this through or before ember-auto-import.

We cannot force the consumer to import graphl from an addon re-export like ember-apollo-client/graphl, because there might be third-party libraries like graphql-tools, that import graphql in turn.

Do you know a clever way for an addon to "inject" arbitrary imports into the host app or control where the host app is getting its imports from?

ef4 commented 5 years ago

(Having multiple instances of ember-auto-import is supposed to be irrelevant. They will find each other and deduplicate, as long as everybody is resolving the same copy of the dependency. I don't think that is actually your problem here.)

graphql-tools has a peerDependency on graphql, which means "if you depend on graphql-tools, you must also depend directly on graphql". So for an app to use graphql-tools, the app must have a dependency on graphql.

ember-apollo-client should do the same thing that graphql-tools does, for the same reason. You should have a peerDependency on graphql. This is the only reliable way to get one shared copy of graphql for the whole app and all its libraries.

This is annoying because it means app authors need to be told to install two packages (ember-apollo-client plus graphql) instead of one (ember-apollo-client). You can help a little by doing it for them in a default blueprint. But it's simply not correct in NPM semantics to expect other packages to be able to see one of your dependencies.

buschtoens commented 5 years ago

Thanks a lot for the pointer. That makes a lot of sense. I'll give it a go later. :+1:

josemarluedke commented 5 years ago

I have tried to add peerDependencies to the PR mentioned above and it doesn't seem to solve the problem we are seeing.

The thing is, we are seeing this error within ember-apollo-client, the consumer in this case is the dummy app.

I have a branch in my fork where it is rebased and has the peer deps specified: https://github.com/josemarluedke/ember-apollo-client/tree/jl-ts-with-peer-deps. Check this commit to see relevant changes.

Any other ideas for us to try?

ef4 commented 5 years ago

There are multiple layers of issues combining here.

That branch you shared allowed me to reproduce a legit bug in ember-auto-import. We are including graphql in both vendor.js and test-support.js. I filed https://github.com/ef4/ember-auto-import/issues/175 to cover that.

That is probably enough to fix your dummy app, because by definition your dummy app and your addon have the same dependencies.

But it's probably not enough to get what you want in real apps, because you still have graphql in dependencies. It needs to not be there. Otherwise you will end up with your own copy that is distinct from the app's copy. Instead you should:

  1. Keep the peerDep on graphql.
  2. Remove the dependency on graphql (because you don't want to bring your own version into people's apps, you want to use their version).
  3. Add a devDependency on graphql (because your dummy app will need to "bring its own" graphql just as regular apps do).
buschtoens commented 5 years ago

Since the original request in this issue is anti-pattern-ish and the underlying bug that motivated it is tracked in #175, I think we can close this issue.

@ef4 Thanks for the suggested peerDep fix! @josemarluedke Thanks for applying it!