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

fix(containers): remove @redwoodjs/api-server's @redwoodjs/internal requirement #8455

Merged
merged 4 commits into from
Jun 1, 2023

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented May 31, 2023

In the last stage of optimized Dockerfiles, you usually see something like this...

RUN --mount=type=cache,target=/root/.yarn/berry/cache \
    --mount=type=cache,target=/root/.cache \
    yarn install --inline-builds && yarn add react react-dom @redwoodjs/api-server @redwoodjs/internal prisma

(Source https://gist.github.com/callingmedic911/15e87c3d26cd4c2c22cad780ff832f75#file-dockerfile-L43-L44).

This instruction only installs the runtime dependencies to keep the final image small. But unfortunately, @redwoodjs/internal is currently a runtime dependency because @redwoodjs/api-server imports a few helper functions from it. @Josh-Walker-GM and I realized this when we derived the experimental @redwoodjs/fastify package from @redwoodjs/api-server, and simply copied over the functions.

@redwoodjs/api-server doesn't list @redwoodjs/internal as a dependency, which is a bug and which is why optimized Dockerfiles have to install it alongside @redwoodjs/api-server manually, but we can use this to our advantage. Removing the need to install @redwoodjs/internal would shave at least ~100 MBs off the last stage of an optimized Dockerfile's size, so this is worth the tradeoff of repeated code.

There's one caveat: the watch binary. The watch binary needs the buildApi function from @redwoodjs/internal, which I'm not going to copy over. But this doesn't matter for the final production stage, and doesn't affect any existing apps because @redwoodjs/api-server never listed @redwoodjs/internal as a dependency in the first place.

This is half true as the api-server's watch bin depends on
internal, but for container deploys that just use serve, watch isn't
necessary.
Moreover, internal hasn't been listed in the package's package.json, so people have to work around this anyway.
@Tobbe
Copy link
Member

Tobbe commented May 31, 2023

This is fine and I have no objections on getting it merged as-is. But I wanted to ask - did you @jtoar (and previously you and @Josh-Walker-GM) consider extracting the methods out into a separate package instead of duplicating them? I think we use the files functions all over the place, so it's possible a lot of other packages would also benefit, if nothing else, by getting clearer dependencies on what they actually need just by looking at something like the nx graph (instead of seeing just a broad dependency towards rwjs/internal).

@replay-io
Copy link

replay-io bot commented May 31, 2023

19 replays were recorded for 8861296.

image 0 Failed
image 19 Passed

View test run on Replay ↗︎

@jtoar
Copy link
Contributor Author

jtoar commented May 31, 2023

@Tobbe I don't remember if we did, I feel like we didn't because it's only these two functions and I'm not sure where we would've put them. These functions feel like they belong in #7039 or some similar introspection package, but I don't want that PR to take on a new life just yet unless it's super clear it'll land, and when Josh has a little less on his plate. Definitely share you concern though and wish we had a solution

Copy link
Member

Tobbe commented May 31, 2023

Thanks, that's all I wanted to hear 🙂

@Josh-Walker-GM
Copy link
Collaborator

I started a more stripped back introspection package a few weeks ago and had got most of the critical stuff working. The issue was I went and experimented with SWC as the parser to produce the AST. If I remember correctly it was troublesome because it had no built-in traversal and there were some issues with no typescript types. I also hadn't decided if SWC was too large a dependency or not.

Anyway, yeah agreed with everyone on this and I haven't forgotten the need for an updated introspection functionality.

Copy link
Member

Tobbe commented May 31, 2023

Interesting that you looked at SWC
This is from the Vite package in my RSC experimental branch
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know how mock this function in a file that has a default export, so I just moved it to a different file

@jtoar jtoar merged commit e73a5b7 into main Jun 1, 2023
9 checks passed
@jtoar jtoar deleted the ds-docker/api-server-remove-need-for-internal branch June 1, 2023 18:11
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 1, 2023
jtoar added a commit that referenced this pull request Jun 1, 2023
…l` requirement (#8455)

* fix(containers): remove @redwoodjs/internal requirement

This is half true as the api-server's watch bin depends on
internal, but for container deploys that just use serve, watch isn't
necessary.
Moreover, internal hasn't been listed in the package's package.json, so people have to work around this anyway.

* add fast glob

* move to a different file for testing
@jtoar jtoar modified the milestones: next-release, next-release-patch, v5.2.4 Jun 2, 2023
jtoar added a commit that referenced this pull request Jun 2, 2023
…l` requirement (#8455)

* fix(containers): remove @redwoodjs/internal requirement

This is half true as the api-server's watch bin depends on
internal, but for container deploys that just use serve, watch isn't
necessary.
Moreover, internal hasn't been listed in the package's package.json, so people have to work around this anyway.

* add fast glob

* move to a different file for testing
@jtoar jtoar modified the milestones: v5.2.4, next-release, next-release-patch Jun 2, 2023
jtoar added a commit that referenced this pull request Jun 2, 2023
…l` requirement (#8455)

* fix(containers): remove @redwoodjs/internal requirement

This is half true as the api-server's watch bin depends on
internal, but for container deploys that just use serve, watch isn't
necessary.
Moreover, internal hasn't been listed in the package's package.json, so people have to work around this anyway.

* add fast glob

* move to a different file for testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants