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

Added "./package.json" to "exports" field #273

Closed

Conversation

bmeeder22
Copy link
Contributor

@bmeeder22 bmeeder22 commented Jan 26, 2022

📦 Pull Request

When importing to react native an error message is appearing:

warn Package @magic-ext/solana has been ignored because it contains invalid configuration. Reason: Package subpath './package.json' is not defined by "exports" in node_modules/@magic-ext/solana/package.json

Not sure if this is causing any issues but it is best practices to export the package.json if exports are defined.

✅ Fixed Issues

Fixes #272

🚨 Test instructions

Add to a react native android app and make sure the warning doesn't appear.

⚠️ Don't forget to add a semver label!

  • patch: Bug Fix

@smithki
Copy link
Contributor

smithki commented Jan 26, 2022

Thanks for you contribution! I can merge this for you, but beforehand we'd need to update package.json files across all packages in the monorepo so we may resolve this issue once and for all. If you don't have the bandwidth for extending your change, we can bring this in-house as well. Let me know!

@@ -20,7 +20,8 @@
"react-native": "./dist/react-native/index.native.js",
"exports": {
"import": "./dist/es/index.mjs",
"require": "./dist/cjs/index.js"
"require": "./dist/cjs/index.js",
"./package.json": "./package.json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a linter warning coming from VSCode's package.json schema-checker. Not sure if this is actually an issue in practice. Do you have any context @bmeeder22?

The warning disappears when conditional "exports" are not in use. I'm not sure if this is a limitation of the spec, or of the schema reference.

Screen Shot 2022-01-25 at 7 55 52 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The place that I am seeing warnings is while attempting to use this package in react-native:

yarn run v1.22.17

$ react-native start
warn Package @magic-ext/solana has been ignored because it contains invalid configuration. Reason: Package subpath './package.json' is not defined by "exports" in /Users/.../mobile/node_modules/@magic-ext/solana/package.json

This appears to be unique to react native >= Node V14 as per this issue: react-native-community/cli#1168

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the warning is in the VS Code linter and not eslint. My guess is that your seeing that error only as part of the VS Code built in linter which might not have the most updated spec.

It appears that this only matters when there are defined exports. In packages without explicitly defined exports everything works just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another good thing to note that the warning appears to just be a warning initializing the extension is working just fine in the RN application. Hopefully that doesn't change in some later version.

@smithki smithki self-requested a review January 26, 2022 03:05
@smithki smithki added the patch Increment the patch version when merged label Jan 26, 2022
@smithki smithki self-assigned this Jan 26, 2022
@smithki smithki changed the title Added package.json to exports Added "./package.json" to "exports" field Jan 26, 2022
@smithki
Copy link
Contributor

smithki commented Jan 26, 2022

Unfortunately our build system complains about this change with the following warning:

Screen Shot 2022-01-26 at 10 50 42 AM

Which leads me to think this is a limitation of the "exports" spec, that conditional exports cannot be mixed at the top-level with path mappings. A format like this works, however:

{
  "exports": {
    "import": {
      ".": "./dist/es/index.mjs",
      "./package.json": "./package.json"
    },
    "require": {
      ".": "./dist/cjs/index.js",
      "./package.json": "./package.json"
    }
  },
}

The problem is that this would require some tinkering with the conventions of our build system, which is pretty risky. We've had issues every single time we've overhauled something in the build system because we have to support so many different platforms, bundlers, and runtimes. Most recently this happened yesterday! (#270). I'd feel more comfortable about approaching a change like this once the Magic team has more cycles. It would be even better if we had some acceptance tests in place beforehand (which is something I'm anticipating for the future).

VSCode complains either way...

Screen Shot 2022-01-26 at 10 53 34 AM


My personal experience with the "exports" spec is unfortunately limited. It took some serious trial & error to get things working as they are with interoperability across web & React Native. I'm tempted to say this is entering wontfix territory for us, as React Native functionality is ultimately unaffected (besides a noisy warning).

If someone wanted to consume a Magic JS package.json file, they could do so with fs.readFile or similar. In a bundling scenario, if it truly is necessary, then the JSON contents could be read at build time and added to an ENV variable in the bundle (in fact, we do something similar here in this repo to read the SDK version at build time).

@smithki
Copy link
Contributor

smithki commented Jan 26, 2022

Closed as wontfix in the short-term. Though I greatly appreciate your efforts, @bmeeder22!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not Exporting package.json in @magic-ext/solana
2 participants