chore: remove external from config #4355
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4355 +/- ##
==========================================
- Coverage 98.69% 98.68% -0.01%
==========================================
Files 205 205
Lines 7329 7328 -1
Branches 2084 2084
==========================================
- Hits 7233 7232 -1
Misses 36 36
Partials 60 60
Continue to review full report at Codecov.
|
| 'stream', | ||
| 'url', | ||
| 'util' | ||
| ], | ||
| input: { | ||
| 'loadConfigFile.js': 'cli/run/loadConfigFile.ts', | ||
| 'rollup.js': 'src/node-entry.ts' |
There was a problem hiding this comment.
I was wondering why fsevents wasn't a problem but it turns out since you extracted the
const moduleName = 'fsevents'
constant in fsevents-importer.ts, the dynamic import can no longer be resolved by Rollup, which is why it does not need to be marked as external. Which is fine. But this also means, we can simplify the interop option below to interop: "default" and get rid of the function.
There was a problem hiding this comment.
"defaultOnly" is just a stricter version of "default", so if "defaultOnly" works, "default" works as well.
There was a problem hiding this comment.
I was wondering why
fseventswasn't a problem but it turns out since you extracted theconst moduleName = 'fsevents'constant in
fsevents-importer.ts, the dynamic import can no longer be resolved by Rollup, which is why it does not need to be marked as external. Which is fine.
I changed the dynamic import back to fsevents, but it's failing on windows and linux as it seems rollup is trying to load fsevents nonetheless, which fails [since it's also not installed]: https://github.com/rollup/rollup/runs/4876116960?check_suite_focus=true is this a possible bug?
I'll revert for the time being.
But this also means, we can simplify the
interopoption below tointerop: "default"and get rid of the function.
Done.
speaking of fsevents, also bumped chokidar.
There was a problem hiding this comment.
I changed the dynamic import back to fsevents, but it's failing on windows and linux
And did you also add "fsevents" back to the external option? Because that is why it was here and why I originally though that externals should be an array containing just "fsevents".
Which I think is ok, i.e. to have your dependencies as "external". We could also employ a technique here used by many other plugins that just import their "package.json" file in their "rollup.config.js" and directly use package.dependencies as input for their external option. That way it "auto-updates" should we ever have additional dependencies.
There was a problem hiding this comment.
Ah, this is the TypeScript plugin complaining, not Rollup. Of course you would need to teach TypeScript about the missing dependency. That was the reason why we had
declare module 'fsevents' {
export default {};
}in declarations.dts, but you did not like that in the past and removed it. You could just add it back, maybe this time with a comment why we have it here.
There was a problem hiding this comment.
Ah, this is the TypeScript plugin complaining, not Rollup.
DOH! 🤦 it got late 😆 ... and confusing, having fsevents installed on my mac with build and tests passing and having the failure only on windows/linux, where fsevents including type information is missing.
Of course you would need to teach TypeScript about the missing dependency. That was the reason why we had
declare module 'fsevents' { export default {}; }
with this PR: #4219 it's possible that the previously used [older?] typescript plugin wasn't quite working, but by bumping it to the newer version the typescript error came up:
from the PR description:
Description bumps rollup-plugin-typescript to latest @rollup/plugin-typescript. there appears to be a problem with the fsevents module and types under linux/windows, as those are optional deps and only installed on macOS. I had to use an any type to get around the typing problem. I'm not sure why and how it worked previously. 🤔
but changed it afterwards by just making the import parameter a variable.
in
declarations.dts, but you did not like that in the past and removed it. You could just add it back, maybe this time with a comment why we have it here.
I checked the history, I [believe] I didn't remove those typings, but let me try if that would work as well on the mac when fsevents (plus typings) is actually installed.
There was a problem hiding this comment.
Ok, I just checked one level of history and you are right 😉
This reverts commit 2ef6c5b.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
ref: #4340 (comment)
marking
fseventsasexternaldidn't seem to be needed as well.@lukastaegert I could not find any references to
SOURCE_DATE_EPOCH. can this likely be removed as well?