bug: Adds file extensions to directories and third party packages

Tjerk-Haaye-Henricus commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Description of the bug

Hey there,

this plugin is nice, but it also adds extensions to directories which breaks things. Instead it should add index.{extension}.

Steps To Reproduce

add to your project
Run
look what happened to directory imports

Expected behavior

It should add index.{extension} at least for esm

Screenshots

No response

Additional context

No response

favna commented 1 year ago

Can you provide a small repository with a reproducible sample so I can properly analyze this?

favna commented 1 year ago

Closing this due to lack of author response

shellscape commented 1 year ago

@favna you can use https://github.com/shellscape/jsx-email/commit/485d472bbc8c97f1075786ace863e8b0961ed658. that commit is in a state where the packages/button/dist contains an example of the issue. this line https://github.com/shellscape/jsx-email/blob/485d472bbc8c97f1075786ace863e8b0961ed658/packages/button/src/button.tsx#L3 contains an import to a directory (for which node resolution will use the index.js barrel file). That gets transformed into var import_utils = require("./utils.js");

To replicate locally, run moon button:build and examine dist/button.js or dist/button.mjs

shellscape commented 1 year ago

This https://github.com/favware/esbuild-plugin-file-path-extensions/blob/dcd70b8023ef25007e770c693dfb81397f30d53a/src/index.ts#L130 is likely where a resolution has to be made. The safest behavior is probably to use https://nodejs.org/api/fs.html#fspromiseslstatpath-options with require.resolve (or import.meta.resolve()) and:

if isFile(), add the extension
else ignore

That will catch scenarios where isDirectory() is true, symlinks and things that are not files, and more importantly ignore what other plugins may do to imports before this one has a chance to examine the resolution.

mikew commented 11 months ago

From a very disgruntled developer trying to turn some simple packages into CJS + ESM, this seems to work for me, but:

I haven't experienced the extension being added to imported dependencies, either with the code below or the plugin in this repo
Totally chokes when you use TypeScript's handy paths

import fs from 'fs'
import path from 'path'
import type { Plugin } from 'esbuild'
import type { Format } from 'tsup'

const VALID_IMPORT_EXTENSIONS = [
  '.js',
  '.jsx',
  '.cjs',
  '.cjsx',
  '.mjs',
  '.mjsx',

  '.ts',
  '.tsx',
  '.cts',
  '.ctsx',
  '.mts',
  '.mtsx',
]

function rewriteImportsPlugin(options: {
  esmExtension: string
  cjsExtension: string
}) {
  const plugin: Plugin = {
    name: 'add-mjs',
    setup(build) {
      const currentBuildFormat: Format | null =
        build.initialOptions.define?.TSUP_FORMAT === '"cjs"'
          ? 'cjs'
          : build.initialOptions.define?.TSUP_FORMAT === '"esm"'
            ? 'esm'
            : null

      if (currentBuildFormat == null) {
        return
      }

      build.onResolve({ filter: /.*/ }, (args) => {
        if (args.kind === 'import-statement') {
          if (!args.path.match(/(^#|\.\/)/)) {
            return
          }

          const desiredExtension =
            currentBuildFormat === 'cjs'
              ? options.cjsExtension
              : currentBuildFormat === 'esm'
                ? options.esmExtension
                : null

          if (desiredExtension == null) {
            return
          }

          let finalName = `${args.path}${desiredExtension}`
          let exactMatch: string | null = null

          for (const ext of VALID_IMPORT_EXTENSIONS) {
            if (
              fs.existsSync(path.join(args.resolveDir, `${args.path}${ext}`))
            ) {
              exactMatch = `${args.path}${ext}`
              break
            }
          }

          if (!exactMatch) {
            finalName = `${args.path}/index${desiredExtension}`
          }

          return { path: finalName, external: true }
        }
      })
    },
  }

  return plugin
}

favna commented 11 months ago

@shellscape I'm having lots of difficulty installing your commit. I don't want to much up my entire development environment with all the junk that your bootstrap script installs* and even after setting up all the dependencies I still get

moon button:build
[ERROR 2023-12-03 10:07:42] log:run  Encountered a critical error, aborting the action pipeline  log.target="moon:action-pipeline:batch:1" log.module_path="moon_action_pipeline::pipeline" log.file="crates/core/action-pipeline/src/pipeline.rs" log.line=216
Error: plugin::create::failed

  × Failed to load and create WASM plugin: 7��import: `env::get_env_var` has not been defined
  ╰─▶ 7��import: `env::get_env_var` has not been defined

I also see that you have completely changed the repo stack since that commit so I assume it's no longer an issue for you. If it still is please let me know and give me a new commit I can use.

I also see that the commit you linked is no longer part of the git history. You seem to have rewritten your git history with a reflog or force push.

*

I use volta, not nvm
I installed pnpm through volta so it's shimmed
I also installed @moonrepo/cli through volta
You had a reference to ${NPM_TOKEN} which I did not have defined and I had to add it manually

favna commented 11 months ago

That all said, I also just released v2.0.0 with various other fixes:

I added the JSX-like extensions listed by @mikew
I added the filter for args.kind === 'import-statement' also listed by @mikew
I fixed the import mapping in package.json, not that big a deal for this package but good to have nonetheless
I marked the package itself as a module so it should run the ESM code always. ESBuild supports this so why not. (tested it in an otherwise CJS project and CJS target to confirm)
I noticed a bug where my cjsExtension was set to .js instead of .cjs as I had always intended. This is fixed and this is why it's 2.0.0.

klippx commented 10 months ago

This is totally fixed and works now, thanks for an AWESOME plugin, it is the solution to my pains with tsup and esm output.

I wish this plugin wasn't needed and that TSUP would simply produce correct output as requested in this feature request https://github.com/egoist/tsup/issues/1058

Take my ⭐️! :)

favna commented 10 months ago

I've also started using it a lot more myself recently in projects such as https://github.com/sapphiredev/framework where we now build completely split CJS and ESM builds and use .cjs and .mjs file extensions so the require statements have to be fixed to be .cjs and import statements have to be fixed to be .mjs.

william-will-angi commented 8 months ago

I am investigating transitioning our build process to use tsup instead of the tsc compiler, and this plugin seems to tie things together. The last issue I'm running into is that for the cjs file output, the index file names are not present causing the imports to break. For example if I have this file path:

- root.ts
- directory-one
  - index.ts

and an import within root.ts:

import { something } from './directory-one'

Then it gets translated to require("./directory-one.cjs")

Where I would expect: require("./directory-one/index.cjs")

I was curious if anyone here had found any eslint plugins or similar to basically force devs to have the full file name. For example something that would fail my build if my import in source code didn't look like:

import { something } from './directory-one/index'

Great plugin and as already mentioned, surprised this isn't built into tsup

mikew commented 8 months ago

@william-will-angi The code I've pasted above doesn't have that issue

favna commented 8 months ago

Yeah I find it quite odd because Sapphire definitely has code like that too and is not facing the issue. Are you sure you're on the latest version of this plugin @william-will-angi ?

william-will-angi commented 8 months ago

I verified with npm ls that I'm using esbuild-plugin-file-path-extensions@2.0.0. It seems to have worked using the plugin that @mikew posted though. Some more context that might be useful:

My tsup.config.ts looks like this:

import { defineConfig } from 'tsup';
import { esbuildPluginFilePathExtensions } from 'esbuild-plugin-file-path-extensions';

}
export default defineConfig([
  {
    entry: ['**/*.{ts,tsx}', '!test/**/*', '!dist/**/*'],
    format: ['cjs'],
    target: 'es2017',
    sourcemap: true,

    esbuildPlugins: [esbuildPluginFilePathExtensions({ cjsExtension: 'cjs' })],
  }
]);

I have "type": "module" in my package.json & my source files are all defined as .ts files.

I can also try to put together a full repro together this evening.

mikew commented 8 months ago

The author of this plugin used the VALID_IMPORT_EXTENSIONS idea I have and another thing, but doesn't do "automatic /index detection". I could PR it, I ended up not using this plugin because extensions are required (in my code, they're optional), and it's small enough to just copy-paste.

favna commented 8 months ago

I would appreciate a PR @mikew

favna commented 6 months ago

As seen above, this should be fully resolved in v2.1.0. If it is not then please chime in and the issue can be re-opened.

pranav-growthx commented 5 months ago

@favna I have a reproduction

https://github.com/pranav-growthx/esbuild-plugin-file-path-extensions-mjs-reproduction

It occurred when I install stuff as peer dependencies

I have also committed the dist folder for you to see

pranav-growthx commented 5 months ago

There seem to be other cases as well. I havent been able to solve for it even after moving stuff from peer dependencies into dependencies in my project (not the one above).

EDIT: Seems like I am not able to go back to the correct builds even in the sample repo

favna commented 5 months ago

EDIT: Seems like I am not able to go back to the correct builds even in the sample repo

Set your package.json to 2.0.0 or ~2.0.0 instead of ^2.1.0. See rules on semver: https://stackoverflow.com/a/22345808/9635150

As for the problem, by any chance are you working on a Windows machine?

pranav-growthx commented 5 months ago

I am building on a mac

pranav-growthx commented 5 months ago

By the correct builds, I meant when I moved express back to dependencies, as it was before when I got a working output (not express.mjs), it still gave me an mjs output when I knew that it originally gave me import express from "express"

Very unsure of that. Same configuration did not produce the same build

favna commented 5 months ago

With your CJS config you set bundle to false but with your ESM build you set it to true. Changing it to false for ESM the import is correct.

By the way considering you're compiling for both ESM and CJS I very very strongly recommend you switch from raw esbuild to tsup (which uses esbuild in the background and this plugin was primarily designed for it) because your current setup will be invalid when analyzing it with https://github.com/arethetypeswrong/arethetypeswrong.github.io because you provide the same typings for for both ESM and CJS. Providing the same typings file is one of the big pitfalls for people starting with cross-compiling.

Fixing the issue of bundling then packing and running attw on the repo the current result is:

npm pack && attw .\gx-editor-1.0.0.tgz

> gx-editor@1.0.0 prepack
> npm run build

> gx-editor@1.0.0 build
> tsc && node scripts/build.mjs

Generating CommonJS build for node
Generating ESM build
npm notice 
npm notice 📦  gx-editor@1.0.0
npm notice === Tarball Contents ===
npm notice 1.2kB dist/cjs/index.js
npm notice 42B   dist/esm/index.mjs
npm notice 11B   dist/types/src/index.d.ts
npm notice 196B  dist/types/tsup.config.d.ts
npm notice 919B  package.json
npm notice === Tarball Details ===
npm notice name:          gx-editor
npm notice version:       1.0.0
npm notice filename:      gx-editor-1.0.0.tgz
npm notice package size:  1.4 kB
npm notice unpacked size: 2.4 kB
npm notice shasum:        2378125bfc2e42426c0c518055aef0d65f752437
npm notice integrity:     sha512-Vle/Cc28j03WC[...]TTVGRk4qWm4YA==
npm notice total files:   5
npm notice
gx-editor-1.0.0.tgz

gx-editor v1.0.0

Build tools:
- @arethetypeswrong/cli@^0.15.3
- typescript@^5.4.5
- esbuild@^0.21.3
- tsup@^8.0.2

💀 Import failed to resolve to type declarations or JavaScript files. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md

┌───────────────────┬──────────────────────┐
│                   │ "gx-editor"          │
├───────────────────┼──────────────────────┤
│ node10            │ 💀 Resolution failed │
├───────────────────┼──────────────────────┤
│ node16 (from CJS) │ 💀 Resolution failed │
├───────────────────┼──────────────────────┤
│ node16 (from ESM) │ 💀 Resolution failed │
├───────────────────┼──────────────────────┤
│ bundler           │ 💀 Resolution failed │
└───────────────────┴──────────────────────┘

Now the bare minimum to fix this mess is to fix the paths in package.json:

-  "main": "index.js",
-  "module": "index.js",
-  "typings": "index.d.ts",
+  "main": "./dist/cjs/index.js",
+  "module": "./dist/esm/index.mjs",
+  "typings": "./dist/types/src/index.d.ts",

However for modern Node it's better to define export mapping:

  "exports": {
    ".": {
      "import": {
        "types": "./dist/types/src/index.d.ts",
        "default": "./dist/esm/index.mjs"
      },
      "require": {
        "types": "./dist/types/src/index.d.ts",
        "default": "./dist/cjs/index.js"
      }
    }
  },

This then results in:

┌───────────────────┬─────────────┐
│                   │ "gx-editor" │
├───────────────────┼─────────────┤
│ node10            │ 🟢          │
├───────────────────┼─────────────┤
│ node16 (from CJS) │ 🟢 (CJS)    │
├───────────────────┼─────────────┤
│ node16 (from ESM) │ 🟢 (CJS)    │
├───────────────────┼─────────────┤
│ bundler           │ 🟢          │
└───────────────────┴─────────────┘

However with tsup this can be a bit nicer:

-  "main": "index.js",
-  "module": "index.js",
-  "typings": "index.d.ts",
+  "main": "./dist/cjs/index.js",
+  "module": "./dist/esm/index.mjs",
+  "typings": "./dist/cjs/index.d.ts",
+  "exports": {
+    ".": {
+      "import": {
+        "types": "./dist/esm/index.d.mts",
+        "default": "./dist/esm/index.mjs"
+      },
+      "require": {
+        "types": "./dist/cjs/index.d.ts",
+        "default": "./dist/cjs/index.js"
+      }
+    }
+  },

The accompanying tsup.config.ts for this is:

import { esbuildPluginFilePathExtensions } from "esbuild-plugin-file-path-extensions";
import { defineConfig, type Options } from "tsup";

const baseOptions: Options = {
  clean: true,
  entry: ["src/**/*{.ts,.tsx}"],
  dts: true,
  minify: false,
  skipNodeModulesBundle: true,
  sourcemap: true,
  target: "es2022",
  tsconfig: "./tsconfig.json",
  keepNames: true,
  esbuildPlugins: [esbuildPluginFilePathExtensions()],
  treeshake: true,
};

export default [
  defineConfig({
    ...baseOptions,
    outDir: "dist/cjs",
    format: "cjs",
  }),
  defineConfig({
    ...baseOptions,
    outDir: "dist/esm",
    format: "esm",
  }),
];

and package.json scripts:

  "scripts": {
    "build": "tsup",
    "prepack": "npm run build",
    "dev": "babel lib --out-dir dist --copy-files --extensions '.ts' --extensions '.tsx' --watch"
  },

also the output JS files dist/cjs/index.js:

'use strict';

var express = require('express');

function _interopDefault (e) { return e && e.__esModule ? e : { default: e }; }

var express__default = /*#__PURE__*/_interopDefault(express);

// src/index.ts
express__default.default();
//# sourceMappingURL=out.js.map
//# sourceMappingURL=index.js.map

and dist/esm/index.mjs:

import express from 'express';

// src/index.ts
express();
//# sourceMappingURL=out.js.map
//# sourceMappingURL=index.mjs.map

pranav-growthx commented 5 months ago

I thought this plugin needed bundle to be set to true to work on the imports

I didnt set bundle to true for cjs cause commonjs require was already working fine due to commonjs supporting directory imports

EDIT: Yup. As soon as I set bundle to false, the plugin stopped working

favna commented 5 months ago

@pranav-growthx can you try version 2.1.1-next.d6bcf76.0?

I added some code that scans your package.json and if the imported path is found in dependencies it is not given a file extension. When I tried it on your repro code both CJS and ESM look good, and in case for ESM both with bundle: true and bundle: false. With bundle: true all the express code gets dumped in dist/esm/index.mjs as per what bundle. With bundle: false the output is simply:

import express from "express";
express();

pranav-growthx commented 5 months ago

Unable to find it

psbakre@Pranavs-MacBook-Air Editor % npm i -D esbuild glob esbuild-plugin-file-path-extensions@2.1.1-next.d6bcf76.0
npm ERR! code ETARGET
npm ERR! notarget No matching version found for esbuild-plugin-file-path-extensions@2.1.1-next.d6bcf76.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/psbakre/.npm/_logs/2024-05-19T14_14_20_487Z-debug-0.log

favna commented 5 months ago

Oh lol my bad, apparently I had those lines disabled in the continuous delivery script. esbuild-plugin-file-path-extensions@2.1.1-next.89d28f7.0 should work.

pranav-growthx commented 5 months ago

I'll check this in some time. Currently dead working. I do have one clarifying question. Does your package.json checking code support

Monorepos where there might be more packages in the parent package.json
Packages like @mui/material also install @mui/system. Will your code also check @mui/system

favna commented 5 months ago

Monorepos where there might be more packages in the parent package.json

No, it checks the package.json at process.cwd() which for monorepos depends where that is based on your mono repo runner so I can't say for sure which it is. I only have experience with Yarn workspaces which I'm pretty sure sets the process.cwd() to the subpackage being processed but since I've never had this issue that you're having I cannot say for sure.

Packages like @mui/material also install @mui/system. Will your code also check @mui/system

No, but it does check all variants of dependencies so you can add @mui/system to your dev deps. Furthermore, while you're perfectly in your right to use raw esbuild for bundling frontend code I very very very strongly implore you to check out Vite which is custom designed for that very purpose and still uses esbuild in the background. Please do not reinvent the wheel, you'll be adding unnecessary maintenance overhead for something that can be achieved in a much simpler way. Lastly, when bundling frontend code please please do NOT bundle it for CJS code. Modern frontend stack (Vite, up-to-date webpack, turbopack, parcel, etc) do not even support CJS anymore because browsers inherently never did. It's completely unnecessary to bundle CJS and again, unnecessary overhead.

See the code here: https://github.com/favware/esbuild-plugin-file-path-extensions/blob/ebd95705f3ed84286a59db38edfacca57521f6ab/src/index.ts#L86-L100

pranav-growthx commented 5 months ago

You are right, and vite was the tool I was initially exploring.

I am building a rich text editor, and the main dependency, lexical, till a few weeks ago exported only commonjs code. Due to which I was only building commonjs.

Recently they have added support for esm. Hence this experiment.

My challenge is that this lib is used as a dependency by other applications. One on the server [cjs] as a headless editor and one on the frontend [esm]. I actually dont want to bundle it since the applications will bundle the code anyway.

With tsc, I dont have an option to change extension to .mjs that I can add both the builds in the same package and it doesnt update the extensions in import

With vite library mode, I need to explicitly mention every package in externals to ensure it doesnt bundle everything [even with bundle set to false]. This by itself isnt an issue but for @lexical/react , I had to explicitly mention every file I was importing from. Hence glob pattern also didnt work

With esbuild, which I am still experimenting with, I found the configuration comparatively simpler and this plugin seems like a good fit but I also understand your concerns here.

Most likely I'd need to restructure my code

Edit: Another reason why i want to build esm and not keep as is is because Next.js's App Router sucks ram worse than chrome when it imports stuff. Since commonjs does not have named imports, you end up with importing everything and then setting up what you need as a variable. Which my editor does for lexical instead of only importing what I need. This prevents us from using the App Router in development due to out of memory crashes and stay on pages router which is completely on maintenance https://github.com/vercel/next.js/issues/54708

KaisSalha commented 5 months ago

@favna I'm still getting this bug unfortunately. here's a small repro https://github.com/KaisSalha/esbuild-plugin-file-path-extensions-bug

favna commented 5 months ago

@KaisSalha Looks like I didn't account for subpath imports in the dependency resolution code. If you can make a PR for that, please be my guest

KaisSalha commented 5 months ago

https://github.com/favware/esbuild-plugin-file-path-extensions/pull/112

favna commented 5 months ago

Released v2.1.2 that includes #112.

(sidenote fun fact, 112 is the European version of 911)

favware / esbuild-plugin-file-path-extensions