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. |
There was a problem hiding this comment.
I didn't notice this change previously, it seems like a possible improvement though 🤔
| }); | ||
| 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?
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
| 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
| 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
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.
9b11d8b to
d90f745
Compare
d90f745 to
822e72f
Compare
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.