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(proxy-server): use tsc to build dist folder #5856

Closed
wants to merge 3 commits into from

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Sep 30, 2021

Summary

Before this change, netlify-cms-proxy-server was built using webpack - which is OK, but not really necessary. This has a few benefits, mentioned originally in #5852:

  • users can import from dist/middlewares/whatever, at their own risk
  • type declarations for free
  • no need to maintain the webpack config anymore! (This is only a benefit because you already have a tsconfig, otherwise you'd be swapping one for the other)
  • remove a bunch of webpack-related dependencies
  • no need to manually shim types in globals.d.ts anymore
  • catch type errors when building (although maybe ts-loader does this already?)

Test plan

Run existing tests - there's no new code, other than some slightly-modified imports.

Implementation

I had to add a "types": "src/index.ts" to netlify-cms-lib-util so that netlify-cms-proxy-server can import from its main module - this seems better to me than importing from netlify-cms-lib-util/src/API anyway. Setting the types to point at src and main to point at dist means that types match up with runtime behaviour. But it could also be worth generating type declaration for lib-util too, down the line. I didn't want to change lib-util much since I'm assuming it has lots of dependencies.

I also had to add the @types/semaphore dev dependency to lib-util, since it relies on semaphore types.

I upgraded netlify-cms-lib-util to a prod dependency in netlify-cms-proxy-server since it's no longer bundled with it.

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • Code is formatted via running yarn format.
  • Tests are passing via running yarn test.
  • The status checks are successful (continuous integration). Those can be seen below.

@mmkal mmkal requested a review from a team September 30, 2021 13:20
@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Sep 30, 2021
@mmkal mmkal marked this pull request as draft September 30, 2021 15:09
@mmkal
Copy link
Contributor Author

mmkal commented Sep 30, 2021

Seems like some cypress tests rely on being able to reach in to the typescript directly. If you would be open to the PR in general, I can look into it and see if there's an easy workaround (simplest thing I can think of is running yarn build before cypress, but maybe that would be an unacceptable slowdown?)

@stale
Copy link

stale bot commented Apr 26, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@mmkal
Copy link
Contributor Author

mmkal commented May 22, 2023

Copying comment #6143 (comment), same thing applies here.

@martinjagodic I'd still like a solution to #6104 but I imagine this is out of date now - are you planning on sticking with webpack + a custom build config? I would have thought in 2023 and with the significant netlify->decap overhaul it'd be better to switch to something like microbundle which can take care of pretty much everything without worrying much about configuration. Or, use turbopack in anticipation of it getting out of beta and becoming the de-facto webpack successor (usually a good idea to bet on Vercel!).

If you disagree, feel free to reopen this and I may be able to find some time to fix up the build.

However, I disagree with @erezrokah's decision to close #5852, we've been relying on this hack for about two years and it's silly, and will be broken if proxy-server starts using express in a slightly different way: #5852 (comment)

So, if I get round to it I might reopen an issue for #5852 to see if the new decap team agrees with me, at some point!

@mmkal mmkal closed this May 22, 2023
@martinjagodic
Copy link
Member

@demshy fyi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants