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

type=module; update dependencies #342

Merged
merged 5 commits into from Jul 30, 2021
Merged

type=module; update dependencies #342

merged 5 commits into from Jul 30, 2021

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Apr 30, 2021

Supersedes part of #436; let’s change that PR into just the mode reducer.

One interesting review note: the caltrain.js test had to be tweaked because it was mixing strings and numbers in the y-scale. In D3 6, the numbers were coerced to strings and considered equivalent; in D3 7, ordinal scales now use InternMap which considers these values to be distinct. I like that the code is more explicit now.

@mbostock mbostock marked this pull request as ready for review July 29, 2021 23:59
@mbostock mbostock requested a review from Fil July 29, 2021 23:59
@mbostock mbostock changed the title type=module type=module; update dependencies Jul 29, 2021
@Fil
Copy link
Contributor

Fil commented Jul 30, 2021

I had the same remark for caltrain, which I had changed differently. I prefer your version (change the data rather than the accessors).

@Fil
Copy link
Contributor

Fil commented Jul 30, 2021

I can't build the bundle:

❯ yarn prepublishOnly
yarn run v1.22.5
$ rm -rf dist && rollup -c
[!] Error: Package subpath './package.json' is not defined by "exports" in /Users/fil/Sites/plot/node_modules/d3/package.json
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './package.json' is not defined by "exports" in /Users/fil/Sites/plot/node_modules/d3/package.json
    at new NodeError (node:internal/errors:278:15)
    at throwExportsNotFound (node:internal/modules/esm/resolve:321:9)
    at packageExportsResolve (node:internal/modules/esm/resolve:546:3)
    at resolveExports (node:internal/modules/cjs/loader:473:36)
    at Function.Module._findPath (node:internal/modules/cjs/loader:513:31)
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:911:27)
    at Function.Module._load (node:internal/modules/cjs/loader:769:27)
    at Module.require (node:internal/modules/cjs/loader:997:19)
    at require (node:internal/modules/cjs/helpers:92:18)
    at Object.<anonymous> (/Users/fil/Sites/plot/rollup.config.js:47:12)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

The error message seems to imply that we have to add "./package.json": "./package.json" in d3's exports, but that in turns creates more issues. I'm not sure what's happening.

@mbostock
Copy link
Member Author

mbostock commented Jul 30, 2021

Strange. You might need Node 14? Actually maybe I didn’t test that step and only ran the tests. I’ll check.

the plot example shows the most common word of length x as a title for each bar

closes #420

(cherry-pick of ea9f8cd)
@Fil
Copy link
Contributor

Fil commented Jul 30, 2021

❯ node -v
v15.5.0

@mbostock
Copy link
Member Author

Yea, I hadn’t run prepublishOnly. I can reproduce and will work on a fix.

@mbostock
Copy link
Member Author

Okay, fixed by using the relative path ./node_modules/d3/package.json, which is a little dirty, but I don’t think there’s a supported way of either resolving the path of a non-exported file (even package.json) or resolving the path to the package directory (i.e., ./node_modules/d3). If there is I’m happy to switch to it but this should work for our purposes, especially since this is only needed for us to publish.

@mbostock mbostock merged commit 13052a8 into main Jul 30, 2021
@mbostock mbostock deleted the mbostock/type-module branch July 30, 2021 21:19
@mbostock mbostock modified the milestone: 0.2 Jul 31, 2021
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.

None yet

2 participants