Allow to build UMD files for packages having dependencies with top-level this in ESM files
#340
Conversation
…vel `this` in ESM files
🦋 Changeset detectedLatest commit: 7cdfd31 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 Report
@@ Coverage Diff @@
## master #340 +/- ##
==========================================
+ Coverage 84.85% 84.89% +0.03%
==========================================
Files 33 33
Lines 1367 1377 +10
Branches 334 338 +4
==========================================
+ Hits 1160 1169 +9
- Misses 202 203 +1
Partials 5 5
Continue to review full report at Codecov.
|
packages/cli/src/build/rollup.ts
Outdated
| case "EMPTY_BUNDLE": | ||
| case "EVAL": | ||
| case "CIRCULAR_DEPENDENCY": | ||
| case "THIS_IS_UNDEFINED": |
There was a problem hiding this comment.
as mentioned in that original issue - throwing for this in source files might still be a good idea, this would complicate thing though, currently, this config factory doesn't have access to the list of all monorepo packages so it cant quite distinguish if this has been reported by Rollup for a dependency of a monorepo package
There was a problem hiding this comment.
We can make it so we only ignore it when type === 'umd' so the errors will happen for the CJS builds(which will always be built)
There was a problem hiding this comment.
Hm, I guess this would work. I was previously concerned about the same error not being reported for some build and reported for others which could confuse users, but when I think about this now - it makes sense and shouldn't really be confusing.
WDYT about making this actually only error for cjs if it's built at all times? This would avoid duplicating this error when you build both CJS and ESM
There was a problem hiding this comment.
WDYT about making this actually only error for cjs if it's built at all times? This would avoid duplicating this error when you build both CJS and ESM
Ok, I see that this is not quite possible because "module" is just built together with "node-dev" type
| @@ -60,7 +81,7 @@ export const rules = { | |||
| .join("\n"); //+ | |||
| // "\n" + | |||
| // "".padEnd(node.tag.loc.start.column); | |||
| if (formatted !== str) { | |||
| if (!compareLines(str, formatted)) { | |||
There was a problem hiding this comment.
this means now that the autofixer will reinsert whitespace again and this will potentially result in many meaningless diffs over time, so maybe lack of whitespace-only lines should be enforced rather than just allowed?
There was a problem hiding this comment.
not sure I understand why this has been changed but I'm all for enforcing one kind of formatting
There was a problem hiding this comment.
I guess this is mainly an issue with my VScode setup that I might even need fixing at some point in time but not sure where this is coming from and I don't have time to tweak this.
Some setting in VSCode (or tools integrated with it) is removing trailing whitespaces for me - this is technically not correct for template literals so that setting has to be some general setting, not knowing about JS-specifics that would turn off this for some lines.
I would expect this kind of a thing to be quite a common setting though so even if I would manage to fix this on my end this might still be a good idea to unify to avoid similar problems for others.
|
@mitchellhamilton I think this is ready for review and merging now. |
Relates to #339