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 comment on default for serverModuleFormat #7536

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented Sep 25, 2023

Since the default for serverModuleFormat is now esm, the comments in the v2 guide for the different configurations of serverBuildTarget were incorrect.

  • Docs

@changeset-bot
Copy link

changeset-bot bot commented Sep 25, 2023

⚠️ No Changeset found

Latest commit: f0c698b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@brophdawg11
Copy link
Contributor

These comments are tricky since they are intended (originally) for v1 users preparing for v2. So CJS is still the default on v1. Now that v2 is released, ESM is the default but there are still 2 types of users reading these docs:

  • Users upgrading to 1.19.x and adopting all future flags to smooth the v2 upgrade
    • Current comments are accurate for these folks
  • Users going straight to v2 without the in-between step
    • Your proposed new comments are accurate for these folks

I think it's probably best to either remove these specific comments or change them to default value in 1.x/default value in 2.x for clarity?

@ngbrown
Copy link
Contributor Author

ngbrown commented Sep 27, 2023

These comments are tricky since they are intended (originally) for v1 users preparing for v2. So CJS is still the default on v1. Now that v2 is released, ESM is the default but there are still 2 types of users reading these docs:

  • Users upgrading to 1.19.x and adopting all future flags to smooth the v2 upgrade
    • Current comments are accurate for these folks

Yes, I considered that, but the document is, at some level, upgrading a user to v2, not to v1.9 with the v2 flags. Users that are upgrading to v1.9 with all flags enabled, would be best served to use the v1.9 documentation.

@brophdawg11
Copy link
Contributor

but the document is, at some level, upgrading a user to v2

If the document were only intended to be read by users upgrading straight to v2, then it wouldn't include the steps of telling them to set future.v2_* flags to true since those flags don't exist in v2. Successfully or not, at the moment it is intentionally trying to be something consumable by both sets of users I mentioned above.

would be best served to use the v1.19 documentation

In an ideal world, yes. But in practice these users are going to land on the main docs and the vast majority of them won't know/remember to switch over to the v1.x docs, so we have to be pragmatic.

Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

Seconding Matt's suggestion here, using default value in 1.x/default value in 2.x would help avoid any confusion

@ngbrown
Copy link
Contributor Author

ngbrown commented Sep 28, 2023

I updated the comments to the following:

serverModuleFormat: "cjs", // default value in 1.x, add before upgrading
...
serverModuleFormat: "esm", // default value in 2.x, can be removed once upgraded

@brophdawg11
Copy link
Contributor

Thank you! I think that's much clearer 👍

@brophdawg11 brophdawg11 merged commit 78fb2f4 into remix-run:main Oct 2, 2023
2 checks passed
@ngbrown ngbrown deleted the docs-v2-patch branch January 19, 2024 22:45
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.

3 participants