Upgrade Rollup, its plugins, Jest and more #492
Conversation
🦋 Changeset detectedLatest commit: d253cbc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #492 +/- ##
==========================================
- Coverage 90.75% 90.52% -0.24%
==========================================
Files 35 35
Lines 1547 1509 -38
Branches 436 428 -8
==========================================
- Hits 1404 1366 -38
+ Misses 137 136 -1
- Partials 6 7 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| {"version":3,"file":"scope-test.umd.min.js","sources":["../src/index.js"],"sourcesContent":["import { x } from \\"somewhere\\";\\nconsole.log(x);\\nexport default \\"something\\";"],"names":["console","log"],"mappings":"qOACAA,QAAQC,wBACO"} | ||
| `); | ||
| ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ dist/scope-test.umd.min.js.map ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ | ||
| {"version":3,"file":"scope-test.umd.min.js","sources":["../src/index.js","../node_modules/somewhere/index.js"],"sourcesContent":["import { x } from \\"somewhere\\";\\nconsole.log(x);\\nexport default \\"something\\";","export let x = process.env.NODE_ENV;"],"names":["console","log"],"mappings":"qOACAA,QAAQC,ICD2B,oBDEpB"} |
There was a problem hiding this comment.
I didn't notice this change previously, it seems like a possible improvement though 🤔
| @@ -1211,6 +1211,6 @@ test("importing css fails with a nice error", async () => { | |||
| "src/blah.css": "", | |||
| }); | |||
| await expect(build(dir)).rejects.toMatchInlineSnapshot( | |||
| `[Error: 🎁 @scope/test only .ts, .tsx, .js, .jsx, and .json files can be imported but "./blah.css" is imported in "src/index.js"]` | |||
| `[Error: 🎁 @scope/test only .ts, .tsx, .js, .jsx, and .json files can be imported but "blah.css" is imported in "src/index.js"]` | |||
There was a problem hiding this comment.
The change is caused by the added path.relative call here: https://github.com/preconstruct/preconstruct/pull/492/files#diff-81fa7127f1a6e4820814f89af91be081857155992595217ccf33830d6116e91eR59
Right now source is already resolved - I don't know how to trace it back to how it was written in the original source code. If we can't trace it back perhaps we should add ./ to computed relative paths if they happen to be in the same directory as the importer?
| expect(err.message).toMatchInlineSnapshot( | ||
| `"🎁 @imports-outside-pkg-dir/pkg-a all relative imports in a package should only import modules inside of their package directory but \\"src/index.js\\" is importing \\"../../some-file\\""` | ||
| ); | ||
| expect(err.message).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
this test currently fails (just like the one here) because resolveId gets called twice for the same thing (with different arguments though, one of the calls seems to see the original location of the source code and not the temp directory used in tests, so it probably has followed a symlink, OTOH testdir helper that is used there doesn't use fixture directories, it writes to temp directly, so I'm not sure what happens there yet, or maybe I've just drawn wrong conclusions and need to recheck this).
The added extension also seems related to this problem - it seems that now resolveId gets called with and without the extension. I'm not sure if something has changed in Rollup or in @rollup/plugin-node-resolve or where. We probably want to just bail out from resolution if we detect that the file lies outside of the pkg dir. It probably has been accomplished before by throwing an error but perhaps now another strategy has to be used or something
| @@ -111,6 +111,7 @@ export function getRollupConfigs(pkg: Package, aliases: Aliases) { | |||
| dir: pkg.directory, | |||
| exports: "named" as const, | |||
| interop, | |||
| esModule: true, | |||
There was a problem hiding this comment.
Rollup has changed the default (here) and I assume that we want to retain __esModule property
| @@ -48,9 +48,6 @@ function writeOutputFile( | |||
| outputFile.map.toString() | |||
| ); | |||
| } | |||
| if (outputOptions.sourcemap !== "hidden") { | |||
There was a problem hiding this comment.
it seems that they now emit this on their own so we don't have to append it manually
| @@ -56,7 +56,10 @@ export default function flowAndNodeDevProdEntry( | |||
| if (!resolved.external && !allowedExtensionRegex.test(resolved.id)) { | |||
| warnings.push( | |||
| new FatalError( | |||
| `only .ts, .tsx, .js, .jsx, and .json files can be imported but "${source}" is imported in ${ | |||
| `only .ts, .tsx, .js, .jsx, and .json files can be imported but "${path.relative( | |||
There was a problem hiding this comment.
both of those added path.relative calls could potentially be hoisted (the call is the same), maybe they even need to be wrapped with normalizePath but I'm not sure if that's required here yet. They might also contain a bug right now because they assume that importer is defined but according to the types (and some of the surrounding code here) it's actually nullable - the tests "pass" though and this doesn't crash. It would be good to add tests covering for the case when the importer is missing.
emmatown
force-pushed
the
rollup-upgrade
branch
3 times, most recently
from
9b11d8b
to
d90f745
Compare
emmatown
force-pushed
the
rollup-upgrade
branch
from
d90f745
to
822e72f
Compare
emmatown
force-pushed
the
rollup-upgrade
branch
from
822e72f
to
0c88d0c
Compare
…ust based on the output
For now, it's just a draft - because Rollup v3 has not been released yet as stable. It's just a PR to get ourselves ready for its release. Since Rollup v3 bumps the node version requirement - we probably should release a new major too if we want to land this right after their release.