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

update esbuild to work with babel #448

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 27 additions & 33 deletions tools/lib/esbuildConfig.js
Copy link
Author

Choose a reason for hiding this comment

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

@dumbmatter when making a second commit to this PR, should I squash my commits?

Copy link
Member

@dumbmatter dumbmatter Jun 2, 2023

Choose a reason for hiding this comment

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

Generally if you're going to squash, you do that when merging (so "you" would be me, since I'm the one who can merge). If you do it while the PR is still in development, it makes it harder to review because I can't easily see what changed since the last time I looked at it.

Original file line number Diff line number Diff line change
@@ -1,58 +1,53 @@
import fs from "fs/promises";
import babel from "@babel/core";
import babelPluginSyntaxTypescript from "@babel/plugin-syntax-typescript";
import babelPluginSportFunctions from "../babel-plugin-sport-functions/index.cjs";
import { getSport } from "./buildFuncs.js";
import babelPluginSportFunctions from "../babel-plugin-sport-functions/index.js";
import getSport from "./getSport.js";
Comment on lines +4 to +5
Copy link
Member

Choose a reason for hiding this comment

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

Why change these lines? Both of these imports are now referencing file that don't exist, so it's an error when you run it.

Copy link
Author

@ldrobner ldrobner May 31, 2023

Choose a reason for hiding this comment

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

I am not sure what is happening here. Something might have changed when I built, but I was unaware of these changes when I made a commit. I will need to revert this change.

Copy link
Member

Choose a reason for hiding this comment

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

Generally you should make sure your master branch is updated to the latest version before making a new branch from it. I recommend you do that and then make these changes again on that new branch, since this is not a big PR.

import path from "path";

const babelCache = {};

// Use babel to run babel-plugin-sport-functions. This is needed because the way bySport is defined, the sport-specific code will run if it's present, which can produce errors. It's not actually needed for isSport.
const pluginSportFunctions = nodeEnv => ({
name: "sport-functions",
setup(build) {
build.onLoad({ filter: /\.tsx?$/, namespace: "file" }, async args => {
build.onResolve({ filter: /\..{1,}Sport/ }, async args => {
Copy link
Member

Choose a reason for hiding this comment

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

The filter runs on the filename, not on the contents of the file, so I think this will never run because no filename matches that regex.

Copy link
Author

Choose a reason for hiding this comment

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

I believe the on resolve callback will be called when we try to 'resolve' the import. So when an import is found in a file, esbuild will try and inline the code from the imported file using our defined callbacks.


This filter will invoke the callback when we find an import with the path field, in the esbuild arguments, equal to .**Sport. This should handle any import for isSport and bySport, I think.


There might be cases where the path field is not .\isSport or .\bySport. Might be cases where path is ..\bySport, bySport, or something similar that could be missed.

Copy link
Author

Choose a reason for hiding this comment

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

what do you think this filter should be?

Copy link
Member

Choose a reason for hiding this comment

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

It's unclear to me what the filter is trying to accomplish. But ultimately, if it works, then I'll figure out why. So make a PR from the latest master so it actually runs and then we'll see :)

return {
path: path.join(args.resolveDir, args.path) + ".ts",
namespace: "by-sport",
};
});

build.onLoad({ filter: /\.tsx?$/, namespace: "by-sport" }, async args => {
const { mtimeMs } = await fs.stat(args.path);
if (babelCache[args.path] && babelCache[args.path].mtimeMs === mtimeMs) {
return babelCache[args.path].result;
}

const isTSX = args.path.endsWith("tsx");

const loader = isTSX ? "tsx" : "ts";
Comment on lines -19 to -21
Copy link
Member

Choose a reason for hiding this comment

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

Why delete this?

const loader = args.path.endsWith("tsx") ? "tsx" : "ts";

const text = await fs.readFile(args.path, "utf8");

// result is undefined if no match, meaning just do normal stuff
let result;
if (
text.includes("bySport") ||
(nodeEnv === "production" && text.includes("isSport"))
) {
const contents = (
await babel.transformAsync(text, {
babelrc: false,
configFile: false,
sourceMaps: "inline",
plugins: [
[babelPluginSyntaxTypescript, { isTSX }],
babelPluginSportFunctions,
],
})
).code;
const contents = (
await babel.transformAsync(text, {
babelrc: false,
configFile: false,
sourceMaps: "inline",
plugins: [
[babelPluginSyntaxTypescript, { isTSX: true }],
babelPluginSportFunctions,
],
})
).code;

result = { contents, loader };
}
const result = { contents, loader };

babelCache[args.path] = {
mtimeMs,
result,
};

if (result === undefined) {
// Might as well return the text, since we have it in memory already
result = { contents: text, loader };
}

return result;
});
},
Expand All @@ -69,16 +64,15 @@ const esbuildConfig = ({ nodeEnv, name }) => {
outfile,
bundle: true,
sourcemap: true,
jsx: "automatic",
jsxDev: nodeEnv === "development",
inject: ["tools/lib/react-shim.mjs"],
Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't exist anymore. Maybe you were basing this on an old version of esbuildConfig.js?

Copy link
Author

Choose a reason for hiding this comment

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

100% could be the case. I think there is something wrong with my fork. I forked the repo way back in 2019, I am sure lots of changes have been made since.

define: {
"process.env.NODE_ENV": JSON.stringify(nodeEnv),
"process.env.SPORT": JSON.stringify(sport),
},
plugins: [pluginSportFunctions(nodeEnv)],

// This is needed because dropbox conditionally requries various node builtins, and esbuild chokes on that even though it never actually makes it to the browser
external: name === "ui" ? ["crypto", "util"] : undefined,
// This is needed because dropbox conditionally requries various node builtins, and esbuild chokes on that even though it never actually makes it to the browser. Skip it for the worker though, otherwise that introduces a spurious error because the type export in worker/index.ts results in a module.exports being added.
platform: name === "ui" ? "node" : undefined,
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

I never had this problem, I am skeptical this change is needed. But the line you deleted is needed, otherwise the build fails for the same reason I wrote in the comment.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure what is happening here. Something might have changed when I built, but I was unaware of these changes when I made a commit. I will need to revert this change.

};
};

Expand Down