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

deps(scale): bump d3-interpolate and d3-scale #1578

Merged
merged 3 commits into from
Dec 22, 2022
Merged

deps(scale): bump d3-interpolate and d3-scale #1578

merged 3 commits into from
Dec 22, 2022

Conversation

jreyes33
Copy link
Contributor

Closes #1577

Bumping these packages allows for d3-color version 3.1.0 or higher to
be installed, fixing this vulnerability:
GHSA-36jr-mh4h-2g58

🐛 Bug Fix

@jreyes33
Copy link
Contributor Author

After bumping the dependency versions, Jest started failing because it doesn't have great support for ES Modules and some d3 packages are now only published as ES Modules (e.g., d3-scale and d3-interpolate).

Adding a regex for those packages in Jest's transformIgnorePatterns is one step of the process, but it's also necessary to have Babel unignore those packages and actually transform them, otherwise we get this error when running yarn jest:

babel-jest: Babel ignores node_modules/internmap/src/index.js - make sure to include the file in Jest's transformIgnorePatterns as well.

Unfortunately, I haven't found a way to override the default ignored files in Babel's config generated by Nimbus. I've tested manually setting the ignore array in the generated babel.config.js to the value below, running Jest without Nimbus and tests pass again. I just don't know how to make this override from Nimbus's config in package.json.

  "ignore": [
    "coverage/",
    "node_modules/(?!(d3-(array|color|format|interpolate|scale|time|time-format)|internmap)/)",
    "public/",
    "esm/",
    "lib/",
    "tmp/",
    "dist/",
    "*.d.ts",
    "__tests__",
    "__mocks__"
  ],

I also included changes to yarn.lock that upgrade @babel/parser to fix this issue that popped up in the process: babel/babel#13581.

@claudiu-muresan-pfa
Copy link

Maybe you can fix it with moduleNameMapper:

 moduleNameMapper: {
    ...
    '^d3$': '<rootDir>/node_modules/d3/dist/d3.min.js',
    "^d3-(.*)$": `<rootDir>/node_modules/d3-$1/dist/d3-$1.min.js`,
     ...
  },

Of course, you can change the above regexps to have only entry.

@jreyes33 when do you think we can have current PR merged?

@claudiu-muresan-pfa
Copy link

@williaster , do you think that current dependencies updates will make it in a future release?

@williaster
Copy link
Collaborator

hey y'all sorry I haven't had a ton of bandwidth to dig into this more. the nimbus layer for managing dev config has proved to be a bit of a headache (was used internally at airbnb for a while but is no longer maintained). we really need to migrate everything off it, which will be a bit of work.

I'll try to see if I can tinker with config overrides, thanks for sharing all the detailed issues you've encountered so far.

@williaster
Copy link
Collaborator

hey @jreyes33 check out #1588 for how to update the babel config.

basically the nimbus babel config in the package.json should end up in the babel.config.js that nimbus generates (it creates these config files via CLI commands, which is why they are .gitignoreed)

@jreyes33
Copy link
Contributor Author

@williaster, just tried that and it's sadly not working. Here's the generated babel.config.js:

module.exports = {
  "ignore": [
    "coverage/",
    "node_modules/",
    "public/",
    "esm/",
    "lib/",
    "tmp/",
    "dist/",
    "*.d.ts",
    "__tests__",
    "__mocks__",
    "node_modules/(?!(d3-(array|color|format|interpolate|scale|time|time-format)|internmap)/)"
  ],
  // …
};

The problem, AFAICT, is that the second item in that array is overeager and ignores the whole node_modules directory; our override ends up having no effect.

This seems to originate from here: https://github.com/airbnb/nimbus/blob/%40airbnb/config-babel%403.1.0/packages/config-babel/src/index.ts#L107

…which in turn comes from here: https://github.com/airbnb/nimbus/blob/%40airbnb/nimbus-common%403.0.1/packages/nimbus-common/src/constants.ts#L11-L20

…so I have no clue how such a constant could be worked around.

@lausek
Copy link

lausek commented Nov 10, 2022

@jreyes33 First of all: Thanks for your effort! This issue has been in our audit logs for quite some time now.

Can't we just add a script that removes the node_modules ignore before start? Something like:

  "scripts": {
    "babel": "yarn run patch-babel && ...",
    "patch-babel": "sed -i /\"node_modules\\\/\"/d babel.config.js"
  }

Note: -i requires an empty string as argument on macos.

@jreyes33
Copy link
Contributor Author

@lausek, I don't know; I'm not a repo maintainer, just the PR author. A maintainer should weigh in.

@williaster
Copy link
Collaborator

Definitely open to patching, not quite sure how to do it though because running nimbus babel ... creates the config and runs babel, so you can't call the patch script in between.

if this is only needed for jest tho, perhaps it would work and we could call it when running the jest scripts. oi so annoying, sorry y'all.

@JayWelsh
Copy link
Contributor

For anyone that requires an immediate workaround for this, this method provided by haydn works wonders: #1577 (comment)

@williaster williaster mentioned this pull request Dec 16, 2022
36 tasks
@williaster
Copy link
Collaborator

@jreyes33 I just landed the migration off nimbus, do you want to try rebasing on top of that and adding the necessary config to the now-committed jest/babel configs?

if you are out of bandwidth let me know and I can take over this PR, you've already done that hard part!

Closes #1577

Bumping these packages allows for `d3-color` version 3.1.0 or higher to
be installed, fixing this vulnerability:
GHSA-36jr-mh4h-2g58
@jreyes33
Copy link
Contributor Author

@williaster, I've updated the PR. Tests passed locally after my last commit updating babel and jest configs: 7c80c8e.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

amazing, thanks so much @jreyes33 – especially for being patient with the nimbus nonsense 🙏

@williaster williaster merged commit 0f7ee0a into airbnb:master Dec 22, 2022
@github-actions
Copy link

🎉 This PR is included in version v2.17.0 of the packages modified 🎉

@williaster
Copy link
Collaborator

ah looks like we didn't update next to handle these imports so the visx site build failed (will make this part of the pull_request CI in the future)

>  yarn dev:demo # or cd packages/visx-demo && yarn dev 
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/@visx/scale/node_modules/d3-scale/src/index.js from /Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/@visx/scale/lib/scales/band.js not supported.
Instead change the require of index.js in /Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/@visx/scale/lib/scales/band.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/@visx/scale/lib/scales/band.js:6:16)
    at Object.<anonymous> (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/@visx/scale/lib/index.js:28:36)
    at eval (webpack-internal:///@visx/scale:1:18)
    at Object.@visx/scale (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:4902:1)
    at __webpack_require__ (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:23:31)
    at eval (webpack-internal:///./src/sandboxes/visx-axis/Example.tsx:11:69)
    at Module../src/sandboxes/visx-axis/Example.tsx (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:4713:1)
    at __webpack_require__ (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:23:31)
    at eval (webpack-internal:///./src/components/Gallery/AxisTile.tsx:5:86)
    at Module../src/components/Gallery/AxisTile.tsx (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:4569:1)
    at __webpack_require__ (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:23:31)
    at eval (webpack-internal:///./src/pages/docs/axis.tsx:11:86)
    at Module../src/pages/docs/axis.tsx (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:4701:1)
    at __webpack_require__ (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:23:31)
    at /Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:91:18
    at Object.<anonymous> (/Users/christopher-williams/dev/visx/packages/visx-demo/.next/server/pages/docs/axis.js:94:10)
    at requirePage (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/require.js:1:1184)
    at loadComponents (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/load-components.js:1:771)
    at DevServer.findPageComponents (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/next-server.js:54:189)
    at DevServer.renderToHTML (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/next-server.js:102:198)
    at DevServer.renderToHTML (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/server/next-dev-server.js:34:590)
    at runMicrotasks (<anonymous>)
    at async DevServer.render (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/next-server.js:52:236)
    at async Object.fn (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/next-server.js:37:264)
    at async Router.execute (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/router.js:23:67)
    at async DevServer.run (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/next-server.js:46:656)
    at async DevServer.handleRequest (/Users/christopher-williams/dev/visx/packages/visx-demo/node_modules/next/dist/next-server/server/next-server.js:17:101) {
  code: 'ERR_REQUIRE_ESM'
}

I tried adding the ignore to the demo's .babelrc but that didn't fix it, and also tried this suggested fix for our next.config.js with the esm packages but that didn't work. @jreyes33 any ideas?

@jreyes33
Copy link
Contributor Author

jreyes33 commented Dec 23, 2022

Updating next to version 12 or higher that comes with ES modules support seems to be the long-term solution, but I tried it quickly and errors in next.config.js pop up. Maybe using next-transpile-modules as suggested here could work in the meantime?

@rj-david
Copy link

rj-david commented Dec 25, 2022

This broke our build due to the ESM requirement after doing a blind npm audit fix

This should be a breaking change and therefore a major version update?

williaster added a commit that referenced this pull request Jan 3, 2023
williaster added a commit that referenced this pull request Jan 3, 2023
williaster added a commit that referenced this pull request Jan 3, 2023
williaster added a commit that referenced this pull request Jan 6, 2023
williaster added a commit that referenced this pull request Jan 6, 2023
…cale`"" (#1621)

* Revert "Revert "deps(scale): bump `d3-interpolate` and `d3-scale` (#1578)" (#1619)"

This reverts commit 75ba46e.

* deps: update yarn.lock

* deps: resolve @babel/parser to ^7.15.3 to fix jest tests
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.

security(visx/scale): Update for d3-color dependencies?
6 participants