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

feat: upgrade esbuild to ^0.21 #374

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

silverwind
Copy link
Contributor

@silverwind silverwind commented Jun 17, 2024

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: #373

@silverwind silverwind changed the title Update esbuild to 0.21 feat: update esbuild to 0.21 Jun 17, 2024
@silverwind silverwind mentioned this pull request Jun 17, 2024
2 tasks
@privatenumber
Copy link
Owner

privatenumber commented Jun 17, 2024

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:

NODE_OPTIONS: --openssl-legacy-provider

@privatenumber privatenumber merged commit 53a5966 into privatenumber:master Jun 17, 2024
1 check passed
@privatenumber privatenumber changed the title feat: update esbuild to 0.21 feat: upgrade esbuild to ^0.21 Jun 17, 2024
@privatenumber
Copy link
Owner

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@silverwind silverwind deleted the esbuild021 branch June 17, 2024 21:34
@silverwind
Copy link
Contributor Author

silverwind commented Jun 17, 2024

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/[email protected]/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/[email protected]/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
Copy link
Owner

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

@silverwind
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

silverwind commented Jun 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why not upgrade esbuild?
2 participants