fix: respect noExternal option with Tsup node

[eric-burel]

Fixes #718

/!\ I haven't tested if it still builds (but I only touched 2 lines of code) + this might be a slight breaking change if some people use tsup-node and accidentally had a  "noExternal" option set (before this PR, the noExternal would be ignored, after this PR, it is correctly considered thus bundling more packages).

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
tsup	✅ Ready (Inspect)	Visit Preview	Oct 6, 2022 at 3:58PM (UTC)

[eric-burel]

@VladimirGrenaderov here is a factored version of the code however I am not sure if it's an improvement, you may want a callback as tiny as possible because it will be called many times.
So in this PR I'd rather stick to fixing tsup-node, and avoid changes that could affect perfs negatively.

Factored version just in case, but again I am not a big fan:

build.onResolve({ filter: /.*/ }, (args) => {
        // Respect explicit options for specific packages
        if (match(args.path, noExternal)) {
          return
        }
        if (match(args.path, external)) {
          return { external: true }
        }
        if (skipNodeModulesBundle) {
          const resolvePatterns = tsconfigPathsToRegExp(
            tsconfigResolvePaths || {}
          )
          // Resolve `paths` from tsconfig
          if (match(args.path, resolvePatterns)) {
            return
          }
          // every other import that looks like a node_modules is external
          if (NON_NODE_MODULE_RE.test(args.path)) {
            return {
              path: args.path,
              external: true,
            }
          }
        }
      })

[koshic]

@eric-burel I meant that 'match(...)' call is not free (regexps as so slow, you know), and we shall not repeat similar calls im multiple callbacks. It's ok for me to have atomic callbacks, but not at the cost of an obvious performance degradation.

Sure, as 'one line fix' this PR is good and much more safer. And it's only package maintainers decision.

[eric-burel]

@VladimirGrenaderov You are totally I right I misunderstood you're initial comment. I've committed a new version that will register only one callback depending on the scenario.

So the initial logic was correct, except for a minor priority issue: anything looking like a node_module was treated as external before the code reached the "noExternal" condition when the skip option was true. This PR aims to fix that and has a more explicit if/else.

Thanks for your help!

[github-actions]

🎉 This PR is included in version 6.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀