Skip to content
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

[RFC] Dependency Upgrades #485

Merged
merged 4 commits into from
Mar 11, 2021
Merged

[RFC] Dependency Upgrades #485

merged 4 commits into from
Mar 11, 2021

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Mar 11, 2021

This PR

I checked for mentions of the const keyword in both the ESM and UMD builds and couldn't find anything so I think our target is successfully locked at ES5.

@drewdaemon drewdaemon removed the request for review from cpgruber March 11, 2021 16:37
@drewdaemon drewdaemon changed the title Bump Karma deps [RFC] Dependency Upgrades Mar 11, 2021
@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #485 (e7aa69d) into master (767307d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #485    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          423       424     +1     
  Lines         5618      5774   +156     
  Branches       871       874     +3     
==========================================
+ Hits          5618      5774   +156     
Impacted Files Coverage Δ
packages/annotations/src/util.ts 100.00% <ø> (ø)
packages/common/src/OperationError.ts 100.00% <ø> (ø)
packages/common/src/request.ts 100.00% <ø> (ø)
...loads/src/portal/portal-export-completion-error.ts 100.00% <ø> (ø)
.../util/merge-pagination/invalid-pagination-input.ts 100.00% <ø> (ø)
packages/downloads/src/remote-server-error.ts 100.00% <100.00%> (ø)
packages/common/src/api.ts 100.00% <0.00%> (ø)
packages/common/src/auth.ts 100.00% <0.00%> (ø)
packages/content/src/hub.ts 100.00% <0.00%> (ø)
packages/events/src/util.ts 100.00% <0.00%> (ø)
... and 212 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 767307d...e7aa69d. Read the comment docs.

Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your recent changes work for me. I would have guessed that you could have achieved the same w/ less changes by setting a karama-typescript config to override the es5 target to es2017, but the work you've done here gets us closer to publishing ES2017 output alongside the ESM/ES5 output, which I appreciate very much.

If this solves the immediate problem, let's merge it as is. If you want to go a step further, you could try removing the rollup TS plugin altogether as I suggested and see if it speeds up the builds.

Either way I'd like to follow up soon w/ a PR to additionally publish ES2017.

@@ -89,7 +89,7 @@ export default {
context: "window",
external: packageNames.concat(arcgisRestJsPackageNames),
plugins: [
typescript(),
typescript({ target: 'es5' }),
Copy link
Member

@tomwayson tomwayson Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gather the reason you changed the rollup plugin is so you could pass in this target. You can't do that w/ rollup-plugin-typescript2?

Either way, it sounds like @rollup/plugin-typescript has come into it's own and may even be faster (would be nice to verify that four our project).

Also, we should explore not even passing the TS to rollup and just having it bundle the esm/es5 output since the UMD bundles are just an artifact needed at publish time (i.e. not used in tests nor needed to be continuously re-built in a watch). That has got to be faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to try to pass the esm build to rollup instead of relying on yet another typescript build.

@drewdaemon
Copy link
Contributor Author

@tomwayson Sounds good. I'll merge.

@drewdaemon drewdaemon merged commit ed01776 into master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants