Fix magic export deprecation messaging #4661

brophdawg11 · 2022-11-21T17:44:09Z

This reverts #3284 due to a few unforeseen issues:

The concatenating of files resulted in duplicate definitions of const getDeprecatedMessage and const warn which caused build failures
The import and actual usage of things in the ESM file broke the ability to tree-shake server-only code out of client bundles:

// Tree shakes correctly as long as things aren't used
export { createCookie } from '@remix-run/node`;

// Doesn't tree shake because `createCookie` is imported and referenced
import * as node from '@remix-run/node';
const createCookie = warn(node.createCookie, ...);

Instead this goes back to the old direct-export approach for the CJS/ESM modules, but keeps the annotated TS @deprecated exports. Then we added a new esbuild plugin that detects imports from remix at build time and logs out a warning for the problematic file:

Testing

Start a fresh remix app
- npx [email protected]
Add a deprecated import in one of your routes
- import { Link } from "remix"
Do a local remix build from this branch and direct the output to your new app:
- REMIX_LOCAL_BUILD_DIRECTORY=../playgrounds/my-remix-app yarn build
Re-run the setup command in your Remix app directory:
- npx remix setup node
npm run dev should show you a build time warning for the deprecated import
TS should show a deprecation error in VScode for the problematic import

changeset-bot · 2022-11-21T17:44:12Z

🦋 Changeset detected

Latest commit: 72a22c9

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

This PR includes changesets to release 16 packages

Name	Type
remix	Patch
@remix-run/dev	Patch
create-remix	Patch
@remix-run/architect	Patch
@remix-run/cloudflare	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/deno	Patch
@remix-run/eslint-config	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/node	Patch
@remix-run/react	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/vercel	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

packages/remix-dev/cli/setup.ts

MichaelDeBoey · 2022-11-21T17:59:40Z

It would be a shame to revert #3284 imo, as we would lose TS deprecations that way, which are another added benefit imo.

Since this only seems to be impacting ESM builds, can we at least keep CJS & TS?

The concatenating of files resulted in duplicate definitions of const getDeprecatedMessage and const warn which caused build failures

This could be fixed by removing it from the specific magicExports file & including it in the concatenated file or we could rename these functions to getDeprecatedMessage_[package-name] & warn_[package-name] to not have duplicate function names 🤔

The import and actual usage of things in the ESM file broke the ability to tree-shake server-only code out of client bundles:

// Tree shakes correctly as long as things aren't used
export { createCookie } from '@remix-run/node`;

// Doesn't tree shake because `createCookie` is imported and referenced
import * as node from '@remix-run/node';
const createCookie = warn(node.createCookie, ...);

So if we can update the ESM imports properly, we can keep these I guess?

@brophdawg11 What if we do the following instead?

import { ${symbol} as ${moduleName}_${symbol} } from '${packageName}';\n
/** @deprecated Import `${symbol}` from `${packageName}` instead. *\n
const ${symbol} = warn(${moduleName}_${symbol}, getDeprecatedMessage('${symbol}', '${packageName}'));\n

So that we have

import { createCookie as node_createCookie } from '@remix-run/node';
/** @deprecated Import `createCookie` from `@remix-run/node` instead. */
const createCookie = warn(node_createCookie, getDeprecatedMessage('createCookie', '@remix-run/node'));

I'm happy to create a PR with the changes I suggested.

brophdawg11 · 2022-11-21T18:35:54Z

@MichaelDeBoey Definitely open to suggestions, this was just the quick fix towards getting 1.8.0-pre.1 out for community testing.

I like the idea of keeping the TS deprecation warnings.

I'm not sure if this would work for ESM, since the import is used and therefore I don't think qualifies for tree-shaking:

import { createCookie as node_createCookie } from '@remix-run/node';
/** @deprecated Import `createCookie` from `@remix-run/node` instead. */
const createCookie = warn(node_createCookie, getDeprecatedMessage('createCookie', '@remix-run/node'));
//                        ^ by accessing the imported thing here, I don't think it'll be tree shaken

Let me see about keeping the types. I think I lean towards keeping the CJS/ESM approaches the same to avoid unintended behavioral differences, but curious what others think there too.

I did the _{packagename} suffix locally as well and it fixes the duplicate identifier issue, but we would also need to enhance the warn function since the react components aren't functions and instead react element objects, so they need to be exported as is instead of called as a function.

…3284)" This reverts commit 848d8b0.

MichaelDeBoey · 2022-11-21T19:07:07Z

I'm not sure if this would work for ESM, since the import is used and therefore I don't think qualifies for tree-shaking:

import { createCookie as node_createCookie } from '@remix-run/node';
/** @deprecated Import `createCookie` from `@remix-run/node` instead. */
const createCookie = warn(node_createCookie, getDeprecatedMessage('createCookie', '@remix-run/node'));
//                        ^ by accessing the imported thing here, I don't think it'll be tree shaken

What about changing it to the following?

import node from '@remix-run/node';
/** @deprecated Import `createCookie` from `@remix-run/node` instead. */
const createCookie = warn(node.createCookie, getDeprecatedMessage('createCookie', '@remix-run/node'));

Let me see about keeping the types. I think I lean towards keeping the CJS/ESM approaches the same to avoid unintended behavioral differences, but curious what others think there too.

This would mean we would loose the benefit of the deprecated JSDoc on all these functions

we would also need to enhance the warn function since the react components aren't functions and instead react element objects, so they need to be exported as is instead of called as a function.

Since our React component are function components, it's basically the same as a normal function, no?
Also: the warn function is re-exporting the same functionality as the original function was doing.

This reverts commit 7774492.

brophdawg11 · 2022-11-21T20:02:42Z

@MichaelDeBoey As soon as we reference anything from @remix-run/node in any way in that bundle, other than a direct re-export, it's unable to be tree shaken and ends up in the client bundle - since the calling of warn is a side effect which breaks tree-shaking:

I added back in the TS deprecations in 5083780 👍

Then @jacob-ebey had a better idea of how we can do this warning at build-time via an esbuild plugin, rather than litter runtime server/browser logs with these warnings (see 229503e). This allows us to better point the user at the file with the problematic import.

I think the React component was potentially from Link being a React.forwardRef - the fn coming into warn was a react-looking object instead of a function in my testing.

MichaelDeBoey

Even though we would loose the extra JSDoc deprecation warnings in your IDE, if there's no other reason to make sure we can still three-shake in ESM, I like the idea of esbuild throwing a warning 👍

MichaelDeBoey · 2022-11-21T20:34:36Z

rollup.utils.js

+        let cjsModule = camelCase(packageName.slice("@remix-run/".length));
+        cjsContents += `var ${cjsModule} = require('${packageName}');\n`;
+        for (let symbol of magicExports.values) {
+          cjsContents +=
+            `Object.defineProperty(exports, '${symbol}', {\n` +
+            "  enumerable: true,\n" +
+            `  get: function () { return ${cjsModule}.${symbol}; }\n` +
+            "});\n";
+        }


I think we can keep the JSDoc deprecation here as well to have an extra IDE warning?

Suggested change

let cjsModule = camelCase(packageName.slice("@remix-run/".length));

cjsContents += `var ${cjsModule} = require('${packageName}');\n`;

for (let symbol of magicExports.values) {

cjsContents +=

`Object.defineProperty(exports, '${symbol}', {\n` +

" enumerable: true,\n" +

` get: function () { return ${cjsModule}.${symbol}; }\n` +

"});\n";

}

cjsContents += `var ${moduleName} = require('${packageName}');\n`;

cjsContents += magicExports.values

.map(

(symbol) =>

`/** @deprecated Import \`${symbol}\` from \`${packageName}\` instead. */\n` +

`exports.${symbol} = ${symbol};\n`

)

.join("\n");

@MichaelDeBoey I'm having trouble actually seeing this JSDoc warning anywhere in my VSCode IDE 🤔

If I create a fresh Remix JS project, VSCode still shows me the deprecation from our .d.ts file in node_modules/remix in my .jsx files:

If I build with this JSDoc comment included and I remove the .d.ts file, then I don't see the deprecation, and I just see the JSDoc from the internal definition of the method, not the JSDoc of the re-exported version from remix:

So for now I'm going to merge this without the CJS JSDoc updates since I can't quite prove that they're picked up. It does seem that even in non-TS projects, at least VScode will pick up our .d.ts deprecation warning.

If we can find specific cases where the JSDoc enhances the DX I'm all for a subsequent PR before we release 1.8.0 stable, but for now I want to get 1.8.0-pre.1 out so folks can help test the new @remix-run/router work inside of Remix 👍

MichaelDeBoey · 2022-11-22T21:47:32Z

@brophdawg11 When doing the following:

Run

npx [email protected] --template remix remix-test-magicExports

Choosing all defaults, except for JavaScript

Run

npm i -D [email protected]
npx remix setup

Disable all ESLint extends
Go to node_modules/remix/dist/index.js

Replace

Object.defineProperty(exports, 'createCookie', {
  enumerable: true,
  get: function () { return node.createCookie; }
});

with

/** @deprecated Import `createCookie` from `@remix-run/node` instead. */
exports.createCookie = node.createCookie;

(which is what my last suggestion was doing)

Add the following line to index.jsx

import {createCookie} from "remix";

const cookieCreator = createCookie; // this is just so my IDE (WebStorm) isn't complaining about `createCookie` not being used

I get the deprecated warning:

If you want, I can create a PR against release-next to add my suggestions again.

* Revert "Deprecate all functions & types exported from `magicExports` (#3284)" This reverts commit 848d8b0. * Single deprecation warning per remix index file * Revert "Single deprecation warning per remix index file" This reverts commit 7774492. * Add back in TS deprecation typings * Warn on deprecated remix imports via esbuild plugin * add chgangeset

remix-cla-bot bot added the CLA Signed label Nov 21, 2022

brophdawg11 commented Nov 21, 2022

View reviewed changes

packages/remix-dev/cli/setup.ts Outdated Show resolved Hide resolved

mcansh approved these changes Nov 21, 2022

View reviewed changes

brophdawg11 added 2 commits November 21, 2022 13:49

Revert "Deprecate all functions & types exported from magicExports (#…

e6a5373

…3284)" This reverts commit 848d8b0.

Single deprecation warning per remix index file

8ae9166

brophdawg11 added 3 commits November 21, 2022 14:42

Revert "Single deprecation warning per remix index file"

12ae44f

This reverts commit 7774492.

Add back in TS deprecation typings

5083780

Warn on deprecated remix imports via esbuild plugin

229503e

brophdawg11 force-pushed the brophdawg11/fix-magic-exports branch from 7774492 to 229503e Compare November 21, 2022 19:55

add chgangeset

72a22c9

brophdawg11 added the package:dev label Nov 21, 2022

mcansh approved these changes Nov 21, 2022

View reviewed changes

MichaelDeBoey approved these changes Nov 21, 2022

View reviewed changes

brophdawg11 merged commit a5fb1f6 into release-next Nov 21, 2022

brophdawg11 deleted the brophdawg11/fix-magic-exports branch November 21, 2022 21:10

remix-run-bot mentioned this pull request Nov 21, 2022

chore: Update version for release (pre) #4663

Merged

MichaelDeBoey mentioned this pull request Nov 22, 2022

chore: add JSDoc deprecation to CJS exports #4673

Merged

mcansh mentioned this pull request Jan 10, 2023

chore: Update version for release (pre) #5050

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix magic export deprecation messaging #4661

Fix magic export deprecation messaging #4661

brophdawg11 commented Nov 21, 2022 •

edited

Loading

changeset-bot bot commented Nov 21, 2022 •

edited

Loading

MichaelDeBoey commented Nov 21, 2022 •

edited

Loading

brophdawg11 commented Nov 21, 2022

MichaelDeBoey commented Nov 21, 2022 •

edited

Loading

brophdawg11 commented Nov 21, 2022

MichaelDeBoey left a comment •

edited

Loading

MichaelDeBoey Nov 21, 2022 •

edited

Loading

brophdawg11 Nov 21, 2022

MichaelDeBoey commented Nov 22, 2022 •

edited

Loading

Fix magic export deprecation messaging #4661

Fix magic export deprecation messaging #4661

Conversation

brophdawg11 commented Nov 21, 2022 • edited Loading

changeset-bot bot commented Nov 21, 2022 • edited Loading

🦋 Changeset detected

MichaelDeBoey commented Nov 21, 2022 • edited Loading

brophdawg11 commented Nov 21, 2022

MichaelDeBoey commented Nov 21, 2022 • edited Loading

brophdawg11 commented Nov 21, 2022

MichaelDeBoey left a comment • edited Loading

Choose a reason for hiding this comment

MichaelDeBoey Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

brophdawg11 Nov 21, 2022

Choose a reason for hiding this comment

MichaelDeBoey commented Nov 22, 2022 • edited Loading

brophdawg11 commented Nov 21, 2022 •

edited

Loading

changeset-bot bot commented Nov 21, 2022 •

edited

Loading

MichaelDeBoey commented Nov 21, 2022 •

edited

Loading

MichaelDeBoey commented Nov 21, 2022 •

edited

Loading

MichaelDeBoey left a comment •

edited

Loading

MichaelDeBoey Nov 21, 2022 •

edited

Loading

MichaelDeBoey commented Nov 22, 2022 •

edited

Loading