privatenumber / esbuild-loader

💠 Speed up your Webpack with esbuild ⚡️
MIT License
3.56k stars 108 forks source link

feat: upgrade esbuild to `^0.21` #374

Closed silverwind closed 3 months ago

silverwind commented 3 months ago

I can't get below two tests to pass locally, maybe the CI has more luck:

✖ esbuild-loader › Webpack 4 › Loader › Keeps dynamic imports by default (441ms)
✖ esbuild-loader › Webpack 5 › Loader › Keeps dynamic imports by default (542ms)

Also, on Node 22, I had to run tests with NODE_OPTIONS=--openssl-legacy-provider to avoid ERR_OSSL_EVP_UNSUPPORTED on webpack 4, which IIRC relies on some deprecated crypto.

Fixes: https://github.com/privatenumber/esbuild-loader/issues/373

privatenumber commented 3 months ago

Thanks for the PR

I can't get below two tests to pass locally, maybe the CI has more luck:

✖ esbuild-loader › Webpack 4 › Loader › Keeps dynamic imports by default (441ms)
✖ esbuild-loader › Webpack 5 › Loader › Keeps dynamic imports by default (542ms)

I'd have to see the actual errors for this to be insightful.

Also, on Node 22, I had to run tests with NODE_OPTIONS=--openssl-legacy-provider to avoid ERR_OSSL_EVP_UNSUPPORTED on webpack 4, which IIRC relies on some deprecated crypto.

Yep, this is expected: https://github.com/privatenumber/esbuild-loader/blob/c257766b8530db5fa373680365e0fd1acff1fa2e/.github/workflows/test.yml#L40

privatenumber commented 3 months ago

:tada: This PR is included in version 4.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

silverwind commented 3 months ago

I'd have to see the actual errors for this to be insightful.

Sorry, here are the errors when I run NODE_OPTIONS=--openssl-legacy-provider pnpm test:

JestAssertionError: expect(received).toBe(expected) // Object.is equality

Expected: 2
Received: 1
    at tests/specs/loader.ts:12:1348
    at runNextTicks (node:internal/process/task_queues:60:5)
    at process.processImmediate (node:internal/timers:449:9)
    at async Y (node_modules/.pnpm/manten@1.2.0/node_modules/manten/dist/index.mjs:2:1368)
✖ esbuild-loader › Webpack 4 › Loader › Keeps dynamic imports by default (574ms)
JestAssertionError: expect(received).toBe(expected) // Object.is equality

Expected: 2
Received: 1
    at tests/specs/loader.ts:12:1348
    at runNextTicks (node:internal/process/task_queues:60:5)
    at process.processImmediate (node:internal/timers:449:9)
    at async Y (node_modules/.pnpm/manten@1.2.0/node_modules/manten/dist/index.mjs:2:1368)
✖ esbuild-loader › Webpack 5 › Loader › Keeps dynamic imports by default (501ms)

node v22.2.0, pnpm v9.4.0

privatenumber commented 3 months ago

Interesting... Would you mind logging the contents of assets here? : https://github.com/privatenumber/esbuild-loader/blob/master/tests/specs/loader.ts#L430

silverwind commented 3 months ago

Sure. By the way the error showed on a clean branch before this PR, so it's definitely not related to this PR.

assets webpack 4
{
  'index.js': CachedSource {
    _source: ConcatSource { children: [Array] },
    _cachedSource: 'module.exports =\n' +
      '/******/ (function(modules) { // webpackBootstrap\n' +
      '/******/ \t// The module cache\n' +
      '/******/ \tvar installedModules = {};\n' +
      '/******/\n' +
      '/******/ \t// The require function\n' +
      '/******/ \tfunction __webpack_require__(moduleId) {\n' +
      '/******/\n' +
      '/******/ \t\t// Check if module is in cache\n' +
      '/******/ \t\tif(installedModules[moduleId]) {\n' +
      '/******/ \t\t\treturn installedModules[moduleId].exports;\n' +
      '/******/ \t\t}\n' +
      '/******/ \t\t// Create a new module (and put it into the cache)\n' +
      '/******/ \t\tvar module = installedModules[moduleId] = {\n' +
      '/******/ \t\t\ti: moduleId,\n' +
      '/******/ \t\t\tl: false,\n' +
      '/******/ \t\t\texports: {}\n' +
      '/******/ \t\t};\n' +
      '/******/\n' +
      '/******/ \t\t// Execute the module function\n' +
      '/******/ \t\tmodules[moduleId].call(module.exports, module, module.exports, __webpack_require__);\n' +
      '/******/\n' +
      '/******/ \t\t// Flag the module as loaded\n' +
      '/******/ \t\tmodule.l = true;\n' +
      '/******/\n' +
      '/******/ \t\t// Return the exports of the module\n' +
      '/******/ \t\treturn module.exports;\n' +
      '/******/ \t}\n' +
      '/******/\n' +
      '/******/\n' +
      '/******/ \t// expose the modules object (__webpack_modules__)\n' +
      '/******/ \t__webpack_require__.m = modules;\n' +
      '/******/\n' +
      '/******/ \t// expose the module cache\n' +
      '/******/ \t__webpack_require__.c = installedModules;\n' +
      '/******/\n' +
      '/******/ \t// define getter function for harmony exports\n' +
      '/******/ \t__webpack_require__.d = function(exports, name, getter) {\n' +
      '/******/ \t\tif(!__webpack_require__.o(exports, name)) {\n' +
      '/******/ \t\t\tObject.defineProperty(exports, name, { enumerable: true, get: getter });\n' +
      '/******/ \t\t}\n' +
      '/******/ \t};\n' +
      '/******/\n' +
      '/******/ \t// define __esModule on exports\n' +
      '/******/ \t__webpack_require__.r = function(exports) {\n' +
      "/******/ \t\tif(typeof Symbol !== 'undefined' && Symbol.toStringTag) {\n" +
      "/******/ \t\t\tObject.defineProperty(exports, Symbol.toStringTag, { value: 'Module' });\n" +
      '/******/ \t\t}\n' +
      "/******/ \t\tObject.defineProperty(exports, '__esModule', { value: true });\n" +
      '/******/ \t};\n' +
      '/******/\n' +
      '/******/ \t// create a fake namespace object\n' +
      '/******/ \t// mode & 1: value is a module id, require it\n' +
      '/******/ \t// mode & 2: merge all properties of value into the ns\n' +
      '/******/ \t// mode & 4: return value when already ns object\n' +
      '/******/ \t// mode & 8|1: behave like require\n' +
      '/******/ \t__webpack_require__.t = function(value, mode) {\n' +
      '/******/ \t\tif(mode & 1) value = __webpack_require__(value);\n' +
      '/******/ \t\tif(mode & 8) return value;\n' +
      "/******/ \t\tif((mode & 4) && typeof value === 'object' && value && value.__esModule) return value;\n" +
      '/******/ \t\tvar ns = Object.create(null);\n' +
      '/******/ \t\t__webpack_require__.r(ns);\n' +
      "/******/ \t\tObject.defineProperty(ns, 'default', { enumerable: true, value: value });\n" +
      "/******/ \t\tif(mode & 2 && typeof value != 'string') for(var key in value) __webpack_require__.d(ns, key, function(key) { return value[key]; }.bind(null, key));\n" +
      '/******/ \t\treturn ns;\n' +
      '/******/ \t};\n' +
      '/******/\n' +
      '/******/ \t// getDefaultExport function for compatibility with non-harmony modules\n' +
      '/******/ \t__webpack_require__.n = function(module) {\n' +
      '/******/ \t\tvar getter = module && module.__esModule ?\n' +
      "/******/ \t\t\tfunction getDefault() { return module['default']; } :\n" +
      '/******/ \t\t\tfunction getModuleExports() { return module; };\n' +
      "/******/ \t\t__webpack_require__.d(getter, 'a', getter);\n" +
      '/******/ \t\treturn getter;\n' +
      '/******/ \t};\n' +
      '/******/\n' +
      '/******/ \t// Object.prototype.hasOwnProperty.call\n' +
      '/******/ \t__webpack_require__.o = function(object, property) { return Object.prototype.hasOwnProperty.call(object, property); };\n' +
      '/******/\n' +
      '/******/ \t// __webpack_public_path__\n' +
      '/******/ \t__webpack_require__.p = "";\n' +
      '/******/\n' +
      '/******/\n' +
      '/******/ \t// Load entry module and return exports\n' +
      '/******/ \treturn __webpack_require__(__webpack_require__.s = 0);\n' +
      '/******/ })\n' +
      '/************************************************************************/\n' +
      '/******/ ([\n' +
      '/* 0 */\n' +
      '/***/ (function(module, __webpack_exports__, __webpack_require__) {\n' +
      '\n' +
      '"use strict";\n' +
      '__webpack_require__.r(__webpack_exports__);\n' +
      'var __create = Object.create;\n' +
      'var __defProp = Object.defineProperty;\n' +
      'var __getOwnPropDesc = Object.getOwnPropertyDescriptor;\n' +
      'var __getOwnPropNames = Object.getOwnPropertyNames;\n' +
      'var __getProtoOf = Object.getPrototypeOf;\n' +
      'var __hasOwnProp = Object.prototype.hasOwnProperty;\n' +
      'var __copyProps = (to, from, except, desc) => {\n' +
      '  if (from && typeof from === "object" || typeof from === "function") {\n' +
      '    for (let key of __getOwnPropNames(from))\n' +
      '      if (!__hasOwnProp.call(to, key) && key !== except)\n' +
      '        __defProp(to, key, { get: () => from[key], enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable });\n' +
      '  }\n' +
      '  return to;\n' +
      '};\n' +
      'var __toESM = (mod, isNodeMode, target) => (target = mod != null ? __create(__getProtoOf(mod)) : {}, __copyProps(\n' +
      '  // If the importer is in node compatibility mode or this is not an ESM\n' +
      '  // file that has been converted to a CommonJS file using a Babel-\n' +
      '  // compatible transform (i.e. "__esModule" has not been set), then set\n' +
      '  // "default" to the CommonJS "module.exports" for node compatibility.\n' +
      '  isNodeMode || !mod || !mod.__esModule ? __defProp(target, "default", { value: mod, enumerable: true }) : target,\n' +
      '  mod\n' +
      '));\n' +
      'var __async = (__this, __arguments, generator) => {\n' +
      '  return new Promise((resolve, reject) => {\n' +
      '    var fulfilled = (value) => {\n' +
      '      try {\n' +
      '        step(generator.next(value));\n' +
      '      } catch (e) {\n' +
      '        reject(e);\n' +
      '      }\n' +
      '    };\n' +
      '    var rejected = (value) => {\n' +
      '      try {\n' +
      '        step(generator.throw(value));\n' +
      '      } catch (e) {\n' +
      '        reject(e);\n' +
      '      }\n' +
      '    };\n' +
      '    var step = (x) => x.done ? resolve(x.value) : Promise.resolve(x.value).then(fulfilled, rejected);\n' +
      '    step((generator = generator.apply(__this, __arguments)).next());\n' +
      '  });\n' +
      '};\n' +
      '/* harmony default export */ __webpack_exports__["default"] = (() => __async(void 0, null, function* () {\n' +
      '  return (yield Promise.resolve().then(() => __toESM(__webpack_require__(1)))).default;\n' +
      '}));\n' +
      '\n' +
      '\n' +
      '/***/ }),\n' +
      '/* 1 */\n' +
      '/***/ (function(module, __webpack_exports__, __webpack_require__) {\n' +
      '\n' +
      '"use strict";\n' +
      '__webpack_require__.r(__webpack_exports__);\n' +
      '/* harmony default export */ __webpack_exports__["default"] = ("test2");\n' +
      '\n' +
      '\n' +
      '/***/ })\n' +
      '/******/ ])["default"];',
    _cachedSize: undefined,
    _cachedMaps: {},
    node: [Function (anonymous)],
    listMap: [Function (anonymous)],
    existsAt: '/dist/index.js',
    emitted: true
  }
}
assets webpack 5
{ 'index.js': SizeOnlySource { _size: 5043 } }
silverwind commented 3 months ago

It might be good to define a build matrix here to test all supported node versions. Node 16 could be dropped in next major release I suppose as it is EOL.

privatenumber commented 3 months ago

That's super strange. I'll look into that code deeper. Are you on Windows?

It might be good to define a build matrix here to test all supported node versions. Node 16 could be dropped in next major release I suppose as it is EOL.

We used to have a matrix, but it starts up a new instance of the VM for each Node version and re-checks out the branch which was a lot slower than just running Node v16 via pnpm.

privatenumber commented 3 months ago

I looked into it and it looks like your esbuild transformation is compiling away the dynamic import(). Wondering if it could be that you had an outdated copy of esbuild or the branch, or be using an outdated version of the build.

Might be obvious but have you done a clean pnpm install?

Also, pnpm test uses the build from dist so you have to run pnpm build first to get the latest copy. pnpm dev loads the src files.

silverwind commented 3 months ago

That was it, after I ran pnpm build, all tests pass. I would recommend running pnpm build implicitely during pnpm test.

In my projects, I do such dependencies with a Makefile, e.g. test depends on build, and build only runs if the source files have changed. Not easily possible with npm scripts because they can't track files, though.