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: allow esbuild config to be overridden #2168

Closed
wants to merge 6 commits into from

Conversation

aiji42
Copy link
Contributor

@aiji42 aiji42 commented Mar 1, 2022

Depending on the values of serverModuleFormat and serverBuildTarget specified in remix.config.js, you can slightly control the configuration of esbuild, but you cannot change the configuration flexibly such as adding plugins.

Next.js uses webpack, and it is possible to change webpack settings from next.config.js. Custom Webpack Config
The webpack function takes the original configuration value as the first argument and the context (server/client decision, webpack version information, etc.) as the second argument, and the configuration value can be overridden through the function.

Remix should also provide a function in remix.config.js that can override the esbuild configuration values, just like Next.js.

  • Docs
  • Tests

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 1, 2022

Hi @aiji42,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

@aiji42 aiji42 changed the title feat: allow esbuild config to be overridden by function in remix.conf… feat: allow esbuild config to be overridden by function in remix.conf.js Mar 1, 2022
@aiji42 aiji42 changed the title feat: allow esbuild config to be overridden by function in remix.conf.js feat: allow esbuild config to be overridden\ Mar 1, 2022
@aiji42 aiji42 changed the title feat: allow esbuild config to be overridden\ feat: allow esbuild config to be overridden Mar 1, 2022
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 1, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@MichaelDeBoey
Copy link
Member

It's intentional that the esbuild config isn't exposed, so that the project isn't tied to esbuild and can be changed to something else whenever the team wants to (they already changed from rollup to esbuild once)

@aiji42
Copy link
Contributor Author

aiji42 commented Mar 2, 2022

@MichaelDeBoey, I understand. However, in reality, there are many needs to rewrite the esbuild configuration. For example, using esbuild-plugin-alias to disable some modules in the build for Cloudflare Workers, specifying jsxFactory for emotion, and so on.
Currently, I'm using patch-package to rewrite the code, but it's very difficult to maintain.

Do you have any plans to address these needs in some way? If you have any other ideas, please let me know. I'll be a contributor.

@aiji42
Copy link
Contributor Author

aiji42 commented Mar 2, 2022

Since v1.2.0, there have been reports that when Cloudflare Workers is set as the runtime, it fails to build when major libraries such as supabase-js and graphql-request are included (#2075). I have already created a PR (#2076) to fix this, but it has not been merged yet.
It is virtually impossible to guarantee that all patterns will work for all the runtimes and libraries used by users. The development community of Remix and we, the users, should quickly share the problems, generalize the issues, and deal with the fixes, but this requires lead time, and in the meantime, users continue to be plagued by bugs.
If it were possible to flexibly change the build settings, it would work very well for minor issues that have a low probability of being encountered, or for cases where users cannot wait for a bug fix. And wouldn't that give you, the development community, more time to develop more effective features?

@MichaelDeBoey
Copy link
Member

@aiji42 I'm not part of the @remix-run team, so I can't tell you more about the plans they're having unfortunately.

I guess you just have to have some patience in order to get your PR merged/declined. 🤷‍♂️

@aiji42
Copy link
Contributor Author

aiji42 commented Mar 2, 2022

@MichaelDeBoey But thanks for your valuable input!
I may not have many peeps to merge, but I will try to get to the point where I can complete this PR 😄

@MichaelDeBoey
Copy link
Member

@aiji42 As I said before: it's intentional to not expose the esbuild config.

I think @ryanflorence or @kentcdodds can give you more info about the exact reasoning behind it and how you could best fix what you want to fix.

@aiji42
Copy link
Contributor Author

aiji42 commented Mar 2, 2022

@ryanflorence @kentcdodds Please let me know what you think.

@aiji42 aiji42 marked this pull request as ready for review March 3, 2022 14:02
@kentcdodds
Copy link
Member

Thanks for putting in the effort @aiji42. This is extremely unlikely to happen. I'm not sure whether @ryanflorence or @mjackson have written out why we don't expose the build config anywhere (I think they have, but can't remember where), but I can say briefly that by exposing the build config it makes it too easy for people to shoot themselves in the foot and limits our ability to make changes that don't break people's apps.

We're much more interested in helping address specific use cases that people have and building in solutions for common use cases to building web apps.

For the edge cases, I know it's probably not what you want to hear, but in the early days of Remix, most of the Remix community made use of https://npm.im/patch-package to get features that Remix hadn't yet implemented and it actually worked out great. It's not something we'd necessarily recommend, but it's a pretty solid solution for folks who need something that we're planning on supporting in the future but isn't ready yet.

@aiji42
Copy link
Contributor Author

aiji42 commented Mar 3, 2022

@kentcdodds Thanks for expressing your thoughts.

So if a certain library cannot be built on Remix, it is practical (though not recommended) to fix it with patch-package?

Do you have any roadmap or rough plans for what features will be added to the compiler in the future? As I contribute to compiler improvement in the future, I would like to have a basis for determining what is acceptable and what is not.

=== Added comments ====

The ability to override esbuild configurations is certainly impactful and dangerous, but are changes acceptable, for example customizable the module alias resolver or to add arbitrary shimming?

@kentcdodds
Copy link
Member

I suggest you create a GitHub discussion for any use cases you have that you feel are not well supported. We do not currently have a public roadmap of features coming (we don't want to announce that a feature is coming and then take over 5 years to deliver on that feature 😬). If you can't find a GitHub discussion for a use case you have then starting up a new one is the best way to get the discussion going and track progress.

@kentcdodds
Copy link
Member

The ability to override esbuild configurations is certainly impactful and dangerous, but are changes acceptable, for example customizable the module alias resolver or to add arbitrary shimming?

I think those are good things to open a new discussion about.

@aiji42
Copy link
Contributor Author

aiji42 commented Mar 4, 2022

@kentcdodds Thank you. I will open a discussion.
Thanks too @MichaelDeBoey!

@aiji42 aiji42 closed this Mar 4, 2022
@jdnichollsc
Copy link

@aiji42 hey mate, did you open a discussion about this? :)

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

Successfully merging this pull request may close these issues.

4 participants