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

Provide hooks to allow customizing ESBuild config #231

Closed

Conversation

crgwbr
Copy link
Contributor

@crgwbr crgwbr commented Jan 27, 2023

This change provides hooks to allow users of the library to customize the projects ESBuild configuration.

For example, assume that a reactivated-based site wanted to use CSS modules. Even though reactivated itself doesn't support this, once this is merged, a site could add config tweak files like this, correlating to packages/reactivated/src/build.client.tsx and packages/reactivated/src/build.renderer.tsx, respectively.

# > client/.reactivated/build.client.ts
import { BuildOptions } from "esbuild";

const classModules = require("esbuild-plugin-class-modules");

export const getBuildConfig = (config: BuildOptions): BuildOptions => {
    config.plugins = config.plugins || [];
    config.plugins.push(classModules());
    return config;
};
# > client/.reactivated/build.renderer.ts
import { BuildOptions } from "esbuild";

const classModules = require("esbuild-plugin-class-modules");

export const getBuildConfig = (config: BuildOptions): BuildOptions => {
    config.plugins = config.plugins || [];
    config.plugins.push(classModules());
    return config;
};

@github-actions
Copy link

github-actions bot commented Jan 27, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@crgwbr
Copy link
Contributor Author

crgwbr commented Jan 27, 2023

I have read the CLA Document and I hereby sign the CLA. If applicable, I have secured permission from my employer.

@crgwbr
Copy link
Contributor Author

crgwbr commented Jan 27, 2023

recheck

silviogutierrez added a commit to getreactivated/contributor-license-agreement that referenced this pull request Jan 27, 2023
@silviogutierrez
Copy link
Owner

@crgwbr : nice! I'll check in a bit, was just working on the project today.

@silviogutierrez
Copy link
Owner

silviogutierrez commented Jan 31, 2023

@crgwbr : this is something I've wanted for a while. At the risk of over engineering it, here are some thoughts:

1. Hooks location / single file

Is it better to have a single reactivated.tsx file somewhere at the root of the client or the like? We already use node_modules/_reactivated as a hidden folder for build artifacts, introducing another feels patchy. This opens up the possibility of other customizations.

2. Other customizations

For example, one hook I need imminently, is the ability to customize the render() call on the server side to inject additional providers. Essentially, the ability to modify this call: https://github.com/silviogutierrez/reactivated/blob/main/packages/reactivated/src/renderer.tsx#L88

Right now, if you wanted to add a server-side rendered Redux store, it's impossible. As the providers are fixed.

3. Discouraging mutation

We already require immer, so should we just pipe the config through immer at this point?

4. Remove the ts-node dependency

I used ts-node for ages, but one of the core reasons I built reactivated was to get rid of the dependency. But like you, I am fanatical about every single file being in typescript and type checked. Since we already use esbuild, it should be trivial for python to detect if this file exists and include it. Bit of a chicken-egg problem but I think it can be thought out.

5. Remove warning

Since the defaults should "just work", I don't think a warning is warranted unless something goes wrong in the inclusion.

Sample file

So ultimately, maybe something like this (the config callable having the benefit of including types for you):

// reactivated.conf.tsx

import React from "react";
import {config} from "@reactivated";

export const {rendererBuildConfig, clientBuildConfig, renderPage} = config({
    clientBuildConfig: (options) => {
        return options;
    },
    rendererBuildConfig: (options) => {
        return options;
    },
    renderPage: (contents) => {
        return <CustomProvider>{contents}</CustomProvider>;
    },
});

Let me know what you think. It's a lot, so I'm happy to help POC some of this.

@crgwbr crgwbr force-pushed the feat_allow_custom_esbuild_config branch 2 times, most recently from 0b7419e to 8ff3986 Compare February 1, 2023 17:05
@crgwbr
Copy link
Contributor Author

crgwbr commented Feb 1, 2023

@silviogutierrez Thanks for the review! I've pushed up some updates that address your notes.

@silviogutierrez
Copy link
Owner

@crgwbr : whoa this solves the chicken/egg problem in a way I hadn't thought of. Actually just putting it in the package! Brilliant. I'll play around with this and comment soon.

We're coupling to npm hard on this (vs pnpm and yarn), but I'm fine with that.

@silviogutierrez silviogutierrez added snapshot Triggers a snapshot release to NPM and PyPI and removed snapshot Triggers a snapshot release to NPM and PyPI labels Feb 2, 2023
@github-actions github-actions bot removed the snapshot Triggers a snapshot release to NPM and PyPI label Feb 2, 2023
@silviogutierrez
Copy link
Owner

silviogutierrez commented Feb 2, 2023

@crgwbr : there's some mypy issues and integration issues when building the full package, so I'll have to take a look.

However, I've published snapshot reactivated@0.28.2-a1334 on NPM and `reactivated==0.28.2a1334 on PyPI that you can use on your own if you want to update your project reqs. These are permanent releases, so they won't disappear.

When I have some time I'll carefully review what's failing. mypy is easy to fix, but the integration (project that actually consumes this without the config override) will need a closer look. Thanks again!

@crgwbr
Copy link
Contributor Author

crgwbr commented Jul 21, 2023

@crgwbr : there's some mypy issues and integration issues when building the full package, so I'll have to take a look.

However, I've published snapshot reactivated@0.28.2-a1334 on NPM and `reactivated==0.28.2a1334 on PyPI that you can use on your own if you want to update your project reqs. These are permanent releases, so they won't disappear.

When I have some time I'll carefully review what's failing. mypy is easy to fix, but the integration (project that actually consumes this without the config override) will need a closer look. Thanks again!

Hey @silviogutierrez, just following up on this. Is there anything I can do to help get this ready for merge?

@crgwbr crgwbr force-pushed the feat_allow_custom_esbuild_config branch from 7c00377 to f7ed4a6 Compare October 18, 2023 16:00
@crgwbr
Copy link
Contributor Author

crgwbr commented Oct 18, 2023

@silviogutierrez I rebased this against main and fixed the merge conflicts. Please let me know what I can do to help get this merged.

@silviogutierrez
Copy link
Owner

@crgwbr : I'm going to revisit this and let you know. I'm running into the need to modify the render function on some projects. Stay tuned.

@silviogutierrez
Copy link
Owner

Working on this now, but in the meantime, check out the patch approach that I've used successfully (and others too):

#280

@silviogutierrez
Copy link
Owner

@crgwbr : I have a branch that should do the trick. Check out PR #312.

You can test it with npm install reactivated@0.36.0-a1926 and pip install reactivated==0.36.0a1926

Create a file in client/ called reactivated.conf.tsx, with the following content:

import {Options} from "reactivated/dist/conf";

export default {
    build: {
        client: (options) => {},
        renderer: (options) => {},
    },
    render: async (content) => {
        return content;
    },
} satisfies Options;

options is a writable draft that you can modify as you see fit.

And run generate_client_assets.

Let me know if you run into any issues!

@crgwbr
Copy link
Contributor Author

crgwbr commented Jan 18, 2024

Thanks, @silviogutierrez!

I'm attempting to upgrade a project to test this change, however the requests version pinning (setup.py#L25-L26) is causing some headaches. Specifically, because of dependencies on chardet, to update the project form reactivated 0.28 to 0.36, I'd also need to downgrade tox from 4.11 -> 3.28.

Considering I've been using reactivated 0.28 with requests 2.31.0 and haven't seen any issues, I think it should be safe to unpin this version now. Though, you may need to add a new constraint for urllib3 = "<2" in order to workaround msabramo/requests-unixsocket#70.

@silviogutierrez
Copy link
Owner

@crgwbr : happy to mess around with the pinning. Can you open a separate PR with these changes and I'll just go ahead and merge them? Not super familiar with the pinning syntax.

@crgwbr
Copy link
Contributor Author

crgwbr commented Jan 18, 2024

@crgwbr : happy to mess around with the pinning. Can you open a separate PR with these changes and I'll just go ahead and merge them? Not super familiar with the pinning syntax.

✅ Done: #314

@silviogutierrez
Copy link
Owner

Fantastic. Merged. Closing this PR in favor of the other.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants