feat(rollup): simplify external #80
Conversation
I noticed that when I upgraded to emotion 10 in one of my packages and started using the babel plugin it swapped `@emotion/styled` for `@emotion/styled-base` which is not marked as an external with this logic and it kinda messed things up. So I improved the external to be what I think is intended.
Codecov Report
@@ Coverage Diff @@
## master #80 +/- ##
==========================================
- Coverage 71.95% 70.75% -1.21%
==========================================
Files 21 21
Lines 378 383 +5
Branches 84 85 +1
==========================================
- Hits 272 271 -1
- Misses 84 88 +4
- Partials 22 24 +2
Continue to review full report at Codecov.
|
|
I will try it out. Somehow this sounds related with the issue I was having earlier this week. Thanks @kentcdodds |
| } | ||
| // for esm/cjs we want to make all node_modules external | ||
| // TODO: support bundledDependencies if someone needs it ever... | ||
| const isNodeModule = id.includes('node_modules') |
There was a problem hiding this comment.
i'd make it more strict with RegExp /\/node_modules\//, u'd have to check if id is normalized here, if not u'd have to construct the regexp based on path.sep (to support windows)
| // TODO: support bundledDependencies if someone needs it ever... | ||
| const isNodeModule = id.includes('node_modules') | ||
| const isRelative = id.startsWith('.') | ||
| return isDep || (!isRelative && !path.isAbsolute(id)) || isNodeModule |
There was a problem hiding this comment.
(!isRelative && !path.isAbsolute(id))
what cases are covered by this which are not covered by isDep || isNodeModule?
There was a problem hiding this comment.
Yeah, it's strange, but this would be called with '@emotion/styled-base' (not listed as a dep because it's added via a babel plugin). So I also need to factor in whether it's relative.
But I can't JUST do relative either because for each mistake it's called with both the source string id and the full path to the resolved module.
There was a problem hiding this comment.
but arent full paths already covered (kinda) by isNodeModule check?
There was a problem hiding this comment.
Full paths are, but not non-relative paths. It's weird I know, but I spent a few hours on this in a project that uses a similar config (paypal-scripts) and this was the only way I found to fix it 🤷♂️
There was a problem hiding this comment.
hm, maybe its brain fart but I still dont get it 😅 non-relative paths can be either:
- package name
- absolute path (which should include node_modules)
- alias (which is not covered, and arguably cant be, here)
am i missing something? 🤔
There was a problem hiding this comment.
It's kinda hard to explain. All I can say is that if I didn't have the relative and absolute logic in here, then weird things would happen :/
There was a problem hiding this comment.
it shouldnt hurt, so im not opposed to it, was rather just curious why this is needed
APPROVED! 👍
|
Thanks friends! |
|
🎉 This PR is included in version 0.49.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What: basically this: Unless you're bundling a
umdfile, all files innode_modulesshould be marked as external. That's what this code does.Why: I noticed that when I upgraded to emotion 10 in one of my packages and started using the babel plugin it swapped
@emotion/styledfor@emotion/styled-basewhich is not marked as an external with this logic and it kinda messed things up. So I improved the external to be what I think is intended.How: the predicate function is called with the same module multiple times which is what made this more tricky than I thought. First it's called with the module id as it appears in the code, then it's called with the full path to the module.
Checklist:
I'd love some input from everyone who's touched the rollup config here. @Andarist, @alexandernanberg, @eliperkins, and @fobbyal. I think this is good, but it'd be nice to get a sanity check. Thanks!