-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(react): provide only umd & esm bundles when packaging #2524
Conversation
tap({ | ||
complete: () => { | ||
updatePackageJson(options, context, target, dependencies); | ||
context.logger.info('Bundle complete.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously, if compilation (or anything else) failed, we'd still generate a dist
folder with a package.json
e649419
to
13b7010
Compare
paths: parsedTSConfig.compilerOptions.paths | ||
} | ||
} | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to invoke the typescript compiler each time, so it can fail the build if something is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. I just wanna check to make sure the type error is only printed once rather than multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the build and it does output the same errors twice (e.g. type errors) if both rollup runs fail the same way. See my other comment for more on this.
compilerOptions: { | ||
rootDir: options.entryRoot, | ||
allowJs: false, | ||
declaration: index === 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only want to emit the declaration files the first time
return { | ||
success: acc.success && result.success | ||
}; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found an issue here with a race condition: if one of the later rollup bundles failed and the first one succeeded, it would still mark the bundle as a success
solution was to separate it into a mergeMap
followed by a scan
--> this way we still keep the parallelism of the bundle generators, but we correctly reduce the final result
I made a stackblitz here to show the issue: https://stackblitz.com/edit/rxjs-cqwcxx
13b7010
to
c28aa16
Compare
10a08b9
to
1156aed
Compare
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior (This is the behavior we have today, before the PR is merged)
When building libraries for the web, we export 3 bundles:
Expected Behavior (This is the new behavior we can expect after the PR is merged)
We don't really need to transpile at all (the consumer of the library can be responsible for that). Instead, we should just be exporting a UMD bundle (for backwards compatibility) and a ESM bundle.
Issue
Closes #2496 - because we don't need to set a
targets
anymore, which works around a babel bug