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

NPM package is imported twice from different sources #33

Closed
red-meadow opened this issue Oct 4, 2023 · 10 comments · Fixed by #119
Closed

NPM package is imported twice from different sources #33

red-meadow opened this issue Oct 4, 2023 · 10 comments · Fixed by #119

Comments

@red-meadow
Copy link

Hi, @JorgenVatle!

I found a subtle but quite critical issue. If some NPM package is imported in both the main app code and a Meteor package, it's imported from different sources. In the first case it's a file processed by Vite, while in the second case it's modules.js file generated by Meteor build system. This causes 2 problems:

  1. 2 versions of the same code are loaded on the client.
  2. If an NPM package has some kind of internal state, the app and the package operate on different states.

I created a repo to illustrate the issue. Here we have a NPM package which has a random value as its internal state. Meteor package and the app code receive different values.

0  console
1  app
2  package

@red-meadow red-meadow mentioned this issue Oct 4, 2023
@JorgenVatle
Copy link
Owner

Thanks for reporting this! 🙏

I ran into similar issues when preparing a React example app. This appears to be the cause for incompatibility between Meteor's React packages and React processed by Vite.

Unfortunately I couldn't find any good solution at the time. Though I imagine we could set up a Vite plugin to point imports for these packages to the Meteor bundle instead of letting Vite compile them.

@red-meadow
Copy link
Author

This appears to be the cause for incompatibility between Meteor's React packages and React processed by Vite.

This is also true for rdb:svelte-meteor-data. It has imports from svelte/internal - and that's how I discovered the issue.

Though I imagine we could set up a Vite plugin to point imports for these packages to the Meteor bundle instead of letting Vite compile them.

So the Vite plugin should:

  1. Detect all imports from NPM packages inside Meteor packages.
  2. If there is the same import in main code, point it to the Meteor bundle.
    Is that correct?

@JorgenVatle
Copy link
Owner

Yeah, that's what I'm thinking would be the best approach to it.

Sorry for getting back to you so late on this.

@red-meadow
Copy link
Author

red-meadow commented Oct 13, 2023

In that case, it should be also taken into account that Meteor package can use Npm.depends() functionality. So the algorytm should be something like:

if (app and package use the same NPM_package) {
    if (package has NPM_package in Npm.depends) {
        if (versions in NPM.depends and package.json are the same) {
             import from Meteor bundle
        }
        else { compile with Vite }
    }
    else { import from Meteor bundle }
}
else { compile with Vite }  

JorgenVatle added a commit that referenced this issue Oct 19, 2023
Rewrites pre-bundling output by Vite to point to Meteor's React bundle instead.

This is primarily as a proof of concept for future reference. There may be a better way to approach the issue.

Ref: #33
@dispbd
Copy link

dispbd commented Jan 10, 2024

Hello @JorgenVatle.

Do we understand correctly that this issue is not closed, since a solution has not yet been found?
We recently decided to test this - in the repo to illustrate we updated all dependencies to the latest versions, but this did not help.

Or maybe we need to make some settings that we may have missed?

@JorgenVatle
Copy link
Owner

Oh, hey, didn't spot your comment until now.

Resolving the issue was a little more complex than I had anticipated and would require a notable amount of refactoring if I recall correctly. Just haven't found the time to get started on that. Externalizing these packages with a custom Vite plugin has been the easiest way to deal with the issue so far. But that assumes that you know which packages are provided by Meteor of course. 😅

I'll have another crack at it later in the evening and get back to you if something comes up. 👍

@durac
Copy link

durac commented Feb 16, 2024

Hey, first of all thanks for the amazing work you do here!
I unfortunately have the same issue with react being imported twice, the workaround in the react example doesn't fix it since after the transformation the chunk is missing some other exports.

Did you already had a chance to look into it again? Is there maybe some other workaround I could try?

@JorgenVatle
Copy link
Owner

JorgenVatle commented Feb 17, 2024

I have a fix in the works now. 😄

Though, since we still need to rely on data made available to us from the Meteor bundle. These npm packages could potentially be outdated or only partially included in the client bundle if Meteor is to do any of tree-shaking. I haven't had the chance to verify whether this is the case, but I suspect it's probably not something you have to worry about.

Either way, just to be safe, we'll make it something you need to opt in to for the time being.

You'll be able to do this on a per-import basis. The syntax will be something like the following:

// No changes here. 
// This will import from your node_modules like you would expect
import MyPackage from 'some-npm-package'

// If you have a Meteor package that already comes bundled with an npm package, the following
// syntax can be used to tell Vite to externalize this import request.
import MyPackage from 'meteor:some-npm-package';

You can import from both places if you find a use case for it. Bearing in mind your client bundle will include both versions

@JorgenVatle
Copy link
Owner

JorgenVatle commented Feb 19, 2024

Bit on an update on this. It does indeed appear like Meteor is lazy-loading npm packages. In the final production bundle, it still might not be an issue as those imports should be picked up on. But in development, it's looking like we need to add explicit exports for each module you need into your Meteor client's mainModule.

With React, I'm not sure how sensitive it is to having multiple versions running. Like, if we pull in a library for React, and this library gets bundled by Vite and not Meteor, would React complain about that? Or a better way to put it, do React libraries ship with a version of React, or is that usually just a peer dependency?

If React libraries use their own versions of the React library, it's probably going to end up being a much better developer experience to simply publish a fork of the react-meteor-data package over npm instead of relying on the Atmosphere-published version. With this approach, we're probably also looking at a considerable improvement in production bundle times as well, since Meteor will no longer be responsible for compiling your React dependencies.

@JorgenVatle
Copy link
Owner

JorgenVatle commented Feb 20, 2024

@durac @red-meadow @dispbd The latest release (meteor-vite@1.10.0 and jorgenvatle:vite-bundler@1.12.9) should resolve the issues you noted. Let me know how it goes. Please do re-open the issue if you're still encountering issues.

@durac I've updated the documentation for React as well as the React example app. Give it a shot and let me know if you run into any issues. 🤞

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

Successfully merging a pull request may close this issue.

4 participants