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

WIP: rework build system to use esbuild. #1043

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

diasbruno
Copy link
Collaborator

@diasbruno diasbruno commented Feb 27, 2024

Fixes #1036.

This is an update to the build system to use esbuild.

It need to:

  • Build a pre-compiled version
  • Build a minified version
  • Use src/index as main module file
  • Run dev/examples server
  • Run tests

let
inherit (nixpkgs) pkgs;
in pkgs.mkShell {
nativeBuildInputs = with pkgs; [jq nodejs_20 swc nodePackages.typescript-language-server];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
nativeBuildInputs = with pkgs; [jq nodejs_20 swc nodePackages.typescript-language-server];
nativeBuildInputs = with pkgs; [jq nodejs_20 nodePackages.typescript-language-server];

@diasbruno
Copy link
Collaborator Author

@doeg What do you think about removing "Build an esm/cjs modules"?

I'm thinking in reducing the amount of artifacts and just a provide a minified version of the library, and, point to main file to "src/index.[tj]s" (this might fix the issue of using fork of this project.

@diasbruno
Copy link
Collaborator Author

We used to pre-compile the library because of some webpack loaders and babel transformers, but I think that if we move to functions and hooks, we can remove build step.

@doeg
Copy link

doeg commented Feb 27, 2024

@diasbruno to tell you the truth, I haven't maintained a React library before and can't give you an informed answer from that perspective. 🤔

Please correct me if I'm wrong: my understanding is that CommonJS modules are typically intended for backend/node imports. I looked at Next.js as an example of server-side imports and it looks like even they support ESM these days. I agree that CommonJS is probably not the best fit for a front-end library like this one.

A question, though! 🙇 If the module distributes just the minified version of the library, wouldn't the entrypoint file still need to use ESM module syntax (or equivalent)?

Comment on lines +26 to +33
"@typescript-eslint/eslint-plugin": "^6.21.0",
"@typescript-eslint/parser": "^6.21.0",
"@webcomponents/custom-elements": "^1.5.0",
"babel-cli": "^6.26.0",
"babel-core": "^6.25.0",
"babel-eslint": "^8.0.1",
"babel-loader": "^7.1.2",
"babel-plugin-add-module-exports": "^0.2.1",
"babel-preset-env": "^1.6.0",
"babel-preset-react": "^6.24.1",
"babel-preset-stage-2": "^6.24.1",
"coveralls": "^3.1.0",
"cross-env": "^5.2.1",
"eslint": "^4.8.0",
"eslint-config-prettier": "^2.6.0",
"eslint-import-resolver-webpack": "^0.9.0",
"eslint-plugin-import": "^2.23.2",
"eslint-plugin-jsx-a11y": "^6.4.1",
"eslint-plugin-prettier": "^2.3.1",
"eslint-plugin-react": "^7.23.2",
"istanbul-instrumenter-loader": "^3.0.0",
"karma": "^6.3.6",
"karma-chrome-launcher": "2.2.0",
"karma-coverage": "^2.0.3",
"karma-firefox-launcher": "1.0.1",
"karma-mocha": "^2.0.1",
"karma-mocha-reporter": "^2.2.1",
"karma-sourcemap-loader": "^0.3.8",
"karma-webpack": "^2.0.4",
"mocha": "^8.4.0",
"npm-run-all": "^4.1.1",
"prettier": "^1.19.1",
"esbuild": "0.20.0",
"esbuild-dev-server": "^0.3.0",
"esbuild-node-externals": "^1.13.0",
"eslint": "^8.56.0",
"eslint-plugin-react": "^7.33.2",
Copy link

Choose a reason for hiding this comment

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

This diff is amazing! 😂

@diasbruno
Copy link
Collaborator Author

diasbruno commented Feb 27, 2024

If the module distributes just the minified version of the library, wouldn't the entrypoint file still need to use ESM module syntax (or equivalent)?

Depends on which version of the language (ES5, ES2015, ...) we compile it to.

If we don't pre-compile the library, users needs to have all the correct languages features to compile (classes, arrows and so on). But, if we move away from class components - I'm guessing, it will no longer be a problem.

I'm thinking on release this as v4-rc1, so we can give it a try and see how that works.

@diasbruno
Copy link
Collaborator Author

I'm going to drop esm/cjs for the first release candidate, so we can test it early.

@doeg
Copy link

doeg commented Feb 28, 2024

Makes sense to me! Let me know if I can be helpful in testing or otherwise. 😸

@rdtabb
Copy link

rdtabb commented Dec 17, 2024

@diasbruno hi! I wonder how the migration to esbuild is going? Can I help?

I really like your lib and have been using it in production at my job for over a year, then I saw this pr, and thought maybe I can be of some use. I have a pretty extensive experience in migrating legacy babel/webpack/plain-ol'-tsc build systems to swc/esbuild/vite-lib-mode etc. I also am quite experienced with maintaining react-js libraries (although all of them are private repos in my company).

So if you can bring me up to speed and maybe offload some work to me, I would be more than happy!

(i don't want to be a nuisance and just try to contribute stuff, who knows, maybe you already got everything figured out, he-he)

@diasbruno
Copy link
Collaborator Author

@rdtabb What made me pause the whole update process is that karma has been deprecated. We can't run the tests using jsdom, since we need to tweak some browser behavior (specially regarding "tabbing" inside the modal).

I've tried a few alternatives, but the process was really stressful.

If you want a really hard quest, you can take this branch (or create another one to give it a try). The goal would be to build and test on a real browser.

If you want to give it a try, I can help/assist with anything you need. Just let me know.

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.

Moving to a "modern" build system (hardcore level)
3 participants