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

Disable Terser Mangle for Turbopack Compatibility #43

Closed
wants to merge 1 commit into from

Conversation

kushadige
Copy link

@kushadige kushadige commented Nov 30, 2024

Summary

This pull request addresses an issue where the Kind object was incorrectly transformed when building the package with Turbopack, specifically when used together with the gql.tada package. Specifically, when using the mangle option in the Terser plugin, the Kind constant was renamed to e, leading to runtime errors due to undefined references ("Kind": () => e).

Turbopack, unlike Webpack, can be sensitive to the mangling of variable names, and this was causing issues with the way Kind was being referenced. By disabling the mangling of variable names, we ensure that the Kind object retains its original name in the final build, making it compatible with Turbopack.

Set of changes

  • Disabled the mangle option in Terser: The mangling of variable names has been turned off to prevent Turbopack from misinterpreting Kind as e.

Impact

  • This change ensures that the Kind object and other constants are not renamed during the build process, making the package work correctly with Turbopack while preserving the intended behavior.
  • No changes to other parts of the package functionality. This change only affects the bundling behavior to ensure compatibility with Turbopack.

How to verify

  • Build the package using Turbopack and verify that no runtime errors occur related to undefined variables.
  • Run the package with both Turbopack and Webpack to ensure compatibility with both bundlers.

Visuals

1. Before the Change (with Mangling enabled):

image image

__TURBOPACK__imported__module__$5b$project$5d2f$node_modules$2f2e$pnpm$2f40$0no$2d$co$2b$graphql$2e$web$40$1$2e$0$2e$11_graphql$40$16$2e$9$2e$0$2f$node_modules$2f40$0no$2d$co$2f$graphql$2e$web$2f$dist$2f$graphql$2e$web$2e$mjs__$5b$app$2d$rsc$5d$__$28$ecmascript$29$__["Kind"] -> references 'e', which is 'undefined' in this case

image

2. After the Change (with Mangling disabled):

image image

__TURBOPACK__imported__module__$5b$project$5d2f$node_modules$2f2e$pnpm$2f40$0no$2d$co$2b$graphql$2e$web$40$1$2e$0$2e$11_graphql$40$16$2e$9$2e$0$2f$node_modules$2f40$0no$2d$co$2f$graphql$2e$web$2f$dist$2f$graphql$2e$web$2e$mjs__$5b$app$2d$rsc$5d$__$28$ecmascript$29$__["Kind"] -> references 'Kind' correctly

Related Discussions/Issues:

ReferenceError: e is not defined error occurs when running next dev --turbo
Error after update to v1.0.10

Copy link

changeset-bot bot commented Nov 30, 2024

⚠️ No Changeset found

Latest commit: 344443e

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

@kitten
Copy link
Member

kitten commented Nov 30, 2024

Even if this addresses the issue, merging and releasing this wouldn't sit right with me. Sorry

To explain the reasons as to why:

  • we don't have a reproduction on any of the original issues
  • we don't have access to a sample of code demonstrating the issue or showing the faulty bundling (and while the screenshots here are helpful, they basically lack the context required, and just roughly demonstrate this)
  • we don't have a response from a member of the Turbopack team or a linked issues addressing their bug
  • changing build outputs to work around a bug in a bundler/transpiler (no matter how popular) isn't appropriate unless there's a clear reason as to why they don't fix the bug or why there's a delay in fixing this bug

Since this points out a clear issue, and implies there's an underlying reproduction here, it should probably be reported with these resources to the Turbopack team. I'm quite sure that if this is an issue, duplicate issues, some maybe even with replies from their team, will exist.

But without a clear reason to work around this, I'm not really sure why this should be changed on our end 😅❤️

@kushadige
Copy link
Author

Even if this addresses the issue, merging and releasing this wouldn't sit right with me. Sorry

To explain the reasons as to why:

  • we don't have a reproduction on any of the original issues
  • we don't have access to a sample of code demonstrating the issue or showing the faulty bundling (and while the screenshots here are helpful, they basically lack the context required, and just roughly demonstrate this)
  • we don't have a response from a member of the Turbopack team or a linked issues addressing their bug
  • changing build outputs to work around a bug in a bundler/transpiler (no matter how popular) isn't appropriate unless there's a clear reason as to why they don't fix the bug or why there's a delay in fixing this bug

Since this points out a clear issue, and implies there's an underlying reproduction here, it should probably be reported with these resources to the Turbopack team. I'm quite sure that if this is an issue, duplicate issues, some maybe even with replies from their team, will exist.

But without a clear reason to work around this, I'm not really sure why this should be changed on our end 😅❤️

It's important not to overlook the fact that Turbopack could potentially have the same issue with other packages in this context. However, from a developer's perspective, it might be a reasonable quick fix to change the naming during the build process (even though the root cause of the issue isn’t directly related to the graphql.web) because it seemed like a small change. The idea was to avoid waiting for a fix from Turbopack, while still allowing us to use it in the gql.tada repository.

That said, I completely agree that it is ultimately up to the team developing the package to decide how they want to handle this, and developers could potentially solve the issue themselves in their own projects using different approaches.

For the sake of providing a concrete example, I've included a simple code snippet below where the e is not defined error occurs. The issue can be resolved by changing the definition of var e = { to Kind in line 1 of node_modules/0no-co/graphql.web/dist/graphql.web.mjs, and exporting it accordingly.

'use client';

import { initGraphQLTada } from 'gql.tada';

export const graphql = initGraphQLTada<{
  introspection: any;
}>();

const PokemonsQuery = graphql(`
  query PokemonsList($limit: Int = 10) {
    pokemons(limit: $limit) {
      id
      name
    }
  }
`);

export default function Home() {
  console.log(PokemonsQuery);

  return <h1>asd</h1>;
}

I'll make sure to report this issue to Turbopack with the necessary details as well. Thanks for your feedback

@kitten
Copy link
Member

kitten commented Nov 30, 2024

Cheers! Just to clarify, if they have acknowledged the bug and can't fix it quickly enough, or if they don't acknowledge it, I'm happy to come back to this and merge the workaround here 👍

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

Successfully merging this pull request may close these issues.

2 participants