Fix package resolution in legacy Node.js #3816
Conversation
colinrotherham
added
🐛 bug
colinrotherham
changed the title
colinrotherham
force-pushed
the
node-require
branch
from
0ae48be to
92c009d
Compare
| - name: Export ${{ matrix.conditions }}, Node.js ${{ matrix.node-version }} | ||
| env: | ||
| # Node.js conditions override from "require" to "import" etc | ||
| # https://nodejs.org/api/cli.html#-c-condition---conditionscondition | ||
| FLAGS: ${{ matrix.conditions != 'require' && format(' --conditions {0}', matrix.conditions) || '' }} | ||
|
|
||
| run: | | ||
| node --eval "console.log(require.resolve('govuk-frontend'))"${{ env.FLAGS }} | ||
| node --eval "console.log(require.resolve('govuk-frontend/package.json'))"${{ env.FLAGS }} | ||
| node --eval "console.log(require.resolve('govuk-frontend/dist/govuk/i18n.js'))"${{ env.FLAGS }} | ||
| node --eval "console.log(require.resolve('govuk-frontend/dist/govuk-esm/i18n.mjs'))"${{ env.FLAGS }} |
There was a problem hiding this comment.
Brilliant! That's going to be neat to check we're not breaking things by changing package.json 😍
packages/govuk-frontend/package.json
Outdated
| "./dist/": "./dist/", | ||
| "./package.json": "./package.json" |
There was a problem hiding this comment.
question: Is there a way we can allow "./" in a similar fashion as "./*"? That'd let us not worry about which folders are specifically accessible 😊
There was a problem hiding this comment.
Ah sorry, I started with that but Visual Studio Code gives me this:
Property ./ is not allowed.
Are there any new paths in v5 we should add for legacy Node.js?
There was a problem hiding this comment.
This was my thinking process by the way:
- Take the compatibility subpath exports from v4.6.0:
"./govuk/": "./govuk/",
"./govuk-esm/": "./govuk-esm/",
"./package.json": "./package.json"- Convert compatibility subpath exports for v5
"./dist/govuk/": "./dist/govuk/",
"./dist/govuk-esm/": "./dist/govuk-esm/",
"./package.json": "./package.json"- Simplify compatibility subpath exports:
"./dist/": "./dist/",
"./package.json": "./package.json"There was a problem hiding this comment.
If you take our package.json files list (what gets published) we have these that can't be resolved:
govuk-prototype-kit.config.json
README.md
There was a problem hiding this comment.
Property ./ is not allowed.
That's shame 😭
If you take our package.json
fileslist (what gets published) we have these that can't be resolved:govuk-prototype-kit.config.json README.md
Could we add them to the list of exports for completedness, please and then we can send it through ⛵
There was a problem hiding this comment.
Mind if I just do govuk-prototype-kit.config.json as Node.js can't require/import Markdown?
There was a problem hiding this comment.
Done it, but let me know
For context Sass does not resolve package.json fields or exports sass/sass#2739 But bundlers like webpack use “style” and “sass” fields using loaders: https://www.npmjs.com/package/css-loader https://www.npmjs.com/package/sass-loader Note: With “exports” present in package.json the stylesheet exports must appear there too to maintain both backwards and forwards compatibility
colinrotherham
force-pushed
the
node-require
branch
from
92c009d to
e36bc25
Compare
We switched to “subpath patterns” recently (supported in Node.js 12.20+) but hadn’t realised that “subpath exports” with trailing slashes (not asterisks) were still needed for older versions We need both flavours because trailing slashes in exports were deprecated in Node.js 16 https://nodejs.org/docs/latest-v18.x/api/deprecations.html#dep0148-folder-mappings-in-exports-trailing- Fixes: #3755
colinrotherham
force-pushed
the
node-require
branch
from
e36bc25 to
00dcfd9
Compare
As described in #3755 this PR fixes point 2)
This may also fix issues with bundlers that also follow Node.js package exports resolution