beaugunderson / ip-address

💻 a library for parsing and manipulating IPv4 and IPv6 addresses in JavaScript
http://ip-address.js.org/
MIT License
582 stars 73 forks source link

Simplified PR for esm and cjs builds #137

Closed pcafstockf closed 3 years ago

pcafstockf commented 3 years ago

This is an alternative PR to #120 My goal was the minimal number of changes that would produce esm and cjs builds.

Basically, tsc is just run twice, with two different tsconfig files.

This article was used as a general guide (no affiliation): How to Create a Hybrid NPM Module for ESM and CommonJS

Both npm test and npm run test-ci still run successfully. I also built ip-address locally, and ran it against my current project using npm link, and all my tests still pass :star:

To summarize this PR... I renamed the root tsconfig file to tsconfig-base and then added a tsconfig file for esm and another for cjs.
The new configs inherit from tsconfig-base and only contain module related overrides. package.json was changed to ensure the correct tsconfig is called, and the new entry points were indicated for cjs vs esm.

FWIW, I worked through these changes for two of my own GitHub projects (pcafstockf/async-injection and pcafstockf/pfta) before starting this PR, and am happy with the results so far.

I used to get warnings from the Angular compiler:

Warning: /src/validations.ts depends on 'ip-address'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

// Repeated for my own two GitHub projects

All 3 of those warnings are now gone.

Caveat

Now I get: Warning: node_modules/ip-address/dist/mjs/lib/ipv4.js depends on 'sprintf-js'. CommonJS or AMD dependencies can cause optimization bailouts.

But that's a problem for another day. Maybe I'll submit a PR to sprintf-js folks when I have a little more time. :smiley:

codecov[bot] commented 3 years ago

Codecov Report

Merging #137 (785f9d0) into master (5d793d4) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #137   +/-   ##
=======================================
  Coverage   98.04%   98.04%           
=======================================
  Files           9        9           
  Lines         564      564           
  Branches       82       82           
=======================================
  Hits          553      553           
  Misses          3        3           
  Partials        8        8           

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 5d793d4...785f9d0. Read the comment docs.

beaugunderson commented 3 years ago

thanks for this @pcafstockf; in looking at this and the other open PR that I didn't quite get merged there are some things I like about both; going to take the best of both now. :)

beaugunderson commented 3 years ago

closed in favor of #138; thanks for the pointer on the hybrid approach! re: sprintf, that can all be ripped out in favor of template literals now :)