fix(bundle): allow importing json with import attributes inside npm packages

[marvinhagemeister]

Running deno bundle on an npm module that imports or requires .json files should work and not error. This PR updates our "check if import attribute is present" logic to exclude files coming from inside npm packages.

Fixes #30263

[nathanwhit]

I think that it needs to be only for require, not imports. It's a little tricky for bundle specifically, because this check happens at module load time, but we only directly know whether it's an import/require in the resolve hook. So I think we need to either add a check at resolve time and disable the one at load, or store the import kind in the resolve hook and somehow associate that with a later load

[dsherret]

Maybe swap these? The in_npm_package check is more expensive.

That said, we should probably make this work even when using cjs outside npm packages and also we still want to surface this error for ESM in npm packages as Nathan mentioned.

[dsherret]

Maybe we should push this error up into the module loader in the CLI and just not bother surfacing it for bundle? Ideally esbuild would handle doing this error for us.

[nathanwhit]

Esbuild just allows importing json without an attribute, as do all other bundlers I'm aware of

[nathanwhit]

I would be in favor of just not erroring at all for bundle, but there's also #30114 which indicates some people want the behavior to align with runtime

[dsherret]

I think we should just not error and close as won’t fix. Probably not worth the effort or added complexity. I’m not sure that pr fully fixes it.

[ramnivas]

As for #30114, it may be okay for deno bundle to accept a wider range of code than plain deno, but not the other way round. For example, if plain deno accepts an npm module, so should deno bundle.

In other words, #30265 (comment) would be okay.

[ramnivas]

I am wondering which way this will go. I see three ways:

1. Run/bundle equivalence:

   deno run foo.ts success <=> deno bundle foo.ts success
   deno run foo.ts failure <=> deno bundle foo.ts failure

   Both succeed or both fail

2. Run's success guarantees bundle's success

   deno run foo.ts success => deno bundle foo.ts success
   deno run foo.ts failure => deno bundle foo.ts success/failure

3. Bundle's success guarantees run's success

   deno bundle foo.ts success => deno run foo.ts success
   deno bundle foo.ts failure => deno run foo.ts success/failure

Option 1 is ideal. But if that's not feasible, option 2 seems more useful since it:

• enables workflows where one starts with deno run and then bundle it
• unlocks using bundler with npm:nodemailer, npm:mine-db etc

I believe this PR chooses option 2.

[nathanwhit]

I am wondering which way this will go. I see three ways:

1. Run/bundle equivalence:
   deno run foo.ts success <=> deno bundle foo.ts success
   deno run foo.ts failure <=> deno bundle foo.ts failure
   Both succeed or both fail
2. Run's success guarantees bundle's success
   deno run foo.ts success => deno bundle foo.ts success
   deno run foo.ts failure => deno bundle foo.ts success/failure
3. Bundle's success guarantees run's success
   deno bundle foo.ts success => deno run foo.ts success
   deno bundle foo.ts failure => deno run foo.ts success/failure

Option 1 is ideal. But if that's not feasible, option 2 seems more useful since it:

• enables workflows where one starts with deno run and then bundle it
• unlocks using bundler with npm:nodemailer, npm:mine-db etc

I believe this PR chooses option 2.

The team discussed this, and we decided we’re going to accept JSON imports without the attribute in deno bundle, but not allow it in deno run. I’ll do a PR this coming week.

[nathanwhit]

Closing in favor of #30413