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

chore: do not use platform: "node" defaults #77

Merged
merged 2 commits into from
Oct 31, 2021

Conversation

lukeed
Copy link
Contributor

@lukeed lukeed commented Oct 31, 2021

The platform config assumes a CommonJS world when set to the "node" value. Any of the implied defaults can be overridden (as already done with format: esm), but I changed the platform value to "neutral" to indicate that (along with these changes) we're no longer relying on any default value from the "node" setting.

Ran the tests locally after new build outputs & everything seems to be fine. Also checked most build output contents manually – most of them appear to be unchanged, but there are some improvements. Using cli-parser as an example here:

Before

var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __markAsModule = (target) => __defProp(target, "__esModule", { value: true });
var __commonJS = (cb, mod) => function __require() {
  return mod || (0, cb[Object.keys(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports;
};
var __reExport = (target, module, desc) => {
  if (module && typeof module === "object" || typeof module === "function") {
    for (let key of __getOwnPropNames(module))
      if (!__hasOwnProp.call(target, key) && key !== "default")
        __defProp(target, key, { get: () => module[key], enumerable: !(desc = __getOwnPropDesc(module, key)) || desc.enumerable });
  }
  return target;
};
var __toModule = (module) => {
  return __reExport(__markAsModule(__defProp(module != null ? __create(__getProtoOf(module)) : {}, "default", module && module.__esModule && "default" in module ? { get: () => module.default, enumerable: true } : { value: module, enumerable: true })), module);
};

// node_modules/mri/lib/index.js
var require_lib = __commonJS({
  "node_modules/mri/lib/index.js"(exports, module) {
    function toArr(any) {

This is all boilerplate stuff that esbuild injects to import & expose CommonJS dependencies as if they were written in ESM format. It's ~30 lines of helper logic that's entirely unneeded because mri has a 'module' entry that can be used.

In the new build, the mri dependency is inlined directly w/o any boilerplate helpers.

This happens in a couple other modules too

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! ✅ I noticed that boilerplate and was wondering how to get rid of it, since I assumed mri, etc would has ES module exports 👍

scripts/build.mjs Outdated Show resolved Hide resolved
@mrbbot mrbbot merged commit 0a538e3 into cloudflare:v2 Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants