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.

Sign up for GitHub

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

Jump to bottom

fix(remix-dev): allow serverBuildPath to end with .cjs #7180

Merged
merged 4 commits into from
Sep 8, 2023
Merged

fix(remix-dev): allow serverBuildPath to end with .cjs #7180

merged 4 commits into from
Sep 8, 2023

Conversation

lpsinger
Copy link
Contributor

Fix a bug that caused the server build file to be emitted into the assets directory if the value of serverBuildPath ended in .cjs.

The bug was due to the server-side portion of the Remix compiler only iterating over files ending in .js and .mjs, but not .cjs.

Note that this diff is smaller than it appears: the largest number of lines affected are in integration/compiler-mjs-cjs-output-test.ts, in which I increased the indentation of the test fixture code in order to parametrize it over file extension.

  • Docs
  • Tests

Testing Strategy:

I found that the existing integration test compiler-mjs-output-test.ts was nearly what I needed. I renamed it to compiler-mjs-cjs-output-test.ts and parametrized it over server build file extension.

Note that this PR both introduces the new test and fixes the bug. To reproduce the test, just revert the part of the diff that affects packages/remix-dev/compiler/server/write.ts and run the integration test.

@changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 3bf8e52

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lpsinger
Copy link
Contributor Author

lpsinger commented Sep 2, 2023

Rebased.

This was referenced Sep 7, 2023
Closed
Closed
@MichaelDeBoey MichaelDeBoey changed the title fix(remix-dev): fix server builds where serverBuildPath ends in .cjs fix(remix-dev): allow serverBuildPath to end with .cjs Sep 7, 2023
@lpsinger @MichaelDeBoey
fix(remix-dev): fix server builds where serverBuildPath ends in .cjs
5a8c372 
Fix a bug that caused the server build file to be emitted into the
assets directory if the value of `serverBuildPath` ended in `.cjs`.

The bug was due to the server-side portion of the Remix compiler
only iterating over files ending in `.js` and `.mjs`, but not
`.cjs`.

Note that this diff is smaller than it appears: the largest number
of lines affected are in integration/compiler-mjs-cjs-output-test.ts,
in which I increased the indentation of the test fixture code in
order to parametrize it over file extension.
MichaelDeBoey
MichaelDeBoey requested changes Sep 7, 2023
View reviewed changes
integration/compiler-mjs-cjs-output-test.ts Outdated Show resolved Hide resolved
integration/compiler-mjs-cjs-output-test.ts Outdated Show resolved Hide resolved
lpsinger and others added 2 commits September 7, 2023 15:26
@lpsinger @MichaelDeBoey
Update integration/compiler-mjs-cjs-output-test.ts
a5ec0ba 
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
@lpsinger @MichaelDeBoey
Update integration/compiler-mjs-cjs-output-test.ts
d91e190 
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
@lpsinger @MichaelDeBoey
Update integration/compiler-mjs-cjs-output-test.ts
3bf8e52 
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
@brophdawg11 brophdawg11 merged commit 70cac93 into remix-run:dev Sep 8, 2023
9 checks passed
@lpsinger lpsinger deleted the server-module-ext-cjs branch September 8, 2023 14:55
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-87234cb-20230909 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.0-pre.10 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@lpsinger
Copy link
Contributor Author

Were you going to backport this to 1.x?

@KarelVerschraegen KarelVerschraegen mentioned this pull request Sep 12, 2023
Closed
@brophdawg11
Copy link
Contributor

I wasn't planning on it - there are a lot of bug fixes included in v2 and we can't backport them all to v1. If there's a compelling reason to we could consider it but otherwise I think this just qualifies as a bug fixed in v2? It's also worth noting that this didn't make the 2.0.0 release branch, so will land in 2.0.1/2.1.0 - whatever comes next.

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-3646f91-20230914 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-e3d5b17-20230916 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot mentioned this pull request Sep 21, 2023
Merged
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.1-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot mentioned this pull request Sep 21, 2023
Merged
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

lpsinger added a commit to lpsinger/gcn.nasa.gov that referenced this pull request Sep 22, 2023
@lpsinger
Convert from CJS to ESM
2ac4da9 
The server builds are still CJS for now. This is made possible by
remix-run/remix#7180, which was included in
the recent release of Remix 2.0.1.
@lpsinger lpsinger mentioned this pull request Sep 22, 2023
Merged
Vidushi-GitHub pushed a commit to Vidushi-GitHub/gcn.nasa.gov that referenced this pull request Oct 11, 2023
@lpsinger @Vidushi-GitHub
Convert from CJS to ESM
81b65be 
The server builds are still CJS for now. This is made possible by
remix-run/remix#7180, which was included in
the recent release of Remix 2.0.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brophdawg11 brophdawg11 brophdawg11 approved these changes

@MichaelDeBoey MichaelDeBoey MichaelDeBoey approved these changes

@mcansh mcansh mcansh approved these changes

@markdalgleish markdalgleish Awaiting requested review from markdalgleish

@pcattori pcattori Awaiting requested review from pcattori

@jacob-ebey jacob-ebey Awaiting requested review from jacob-ebey

Assignees
No one assigned
Labels
CLA Signed package:dev
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lpsinger @brophdawg11 @MichaelDeBoey @mcansh