dividab / tsconfig-paths

Load node modules according to tsconfig paths, in run-time or via API.
MIT License
1.82k stars 104 forks source link

For review of #135 #139

Closed cspotcode closed 2 years ago

cspotcode commented 4 years ago

This PR is meant to be #135, but I had to push to a different branch.

codecov[bot] commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@2461cc9). Click here to learn what that means. The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #139   +/-   ##
========================================
  Coverage          ?   0.00%           
========================================
  Files             ?       4           
  Lines             ?     129           
  Branches          ?      52           
========================================
  Hits              ?       0           
  Misses            ?     129           
  Partials          ?       0           
Impacted Files Coverage Δ
src/match-path-async.ts 0.00% <0.00%> (ø)
src/match-path-sync.ts 0.00% <0.00%> (ø)
src/options.ts 0.00% <0.00%> (ø)
src/filesystem.ts 0.00% <0.00%> (ø)

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 2461cc9...97f7f87. Read the comment docs.

jonaskello commented 4 years ago

Thanks for picking this up :-) LGTM

jonaskello commented 4 years ago

@cspotcode I can add you to npm too so you can publish if you want?

cspotcode commented 4 years ago

That'd be great, thanks.

Should this be a patch release or a major release? Anything I need to know about the release process, extra steps, changelog tooling, stuff like that?

On Mon, Aug 10, 2020, 5:16 PM Jonas Kello notifications@github.com wrote:

@cspotcode https://github.com/cspotcode I can add you to npm too so you can publish if you want?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dividab/tsconfig-paths/pull/139#issuecomment-671593883, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC35OA7HPTEVQ4NPI5MRQ3SABPRXANCNFSM4P2HM5GA .

jonaskello commented 4 years ago

The changelog is manually updated. Just move the unreleased heading to new version heading before publishing (and remember to make a compare link like for the other version headings, and update the unreleased link too). I usually do this directly on master. If you have ideas for automating this more I'm open for that :-).

It has been a while since this package was published but I think the instructions in the readme still apply. So basically just do yarn version and the preversion, postversion scripts should do the publishing.

I guess the correct semantic versioning would be a patch since it is basically a bugfix. However it may (unintentionally) break existing consumer so for that reason perhaps a major version is called for. I guess it can even be viewed as a new feature that we support other extensions which would make it a minor release. I don't really have a strong opinion on the version increment for this so you can pick what you think is appropriate.

I've added you to npm so you should be able to publish too now :-).

jonaskello commented 3 years ago

@cspotcode Is this ready to be merged?

cspotcode commented 3 years ago

Not sure, I've got my hands full with ts-node and realistically won't have time for this in the near future.

On Tue, Jul 6, 2021, 11:08 AM Jonas Kello @.***> wrote:

@cspotcode https://github.com/cspotcode Is this ready to be merged?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dividab/tsconfig-paths/pull/139#issuecomment-874843489, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC35OCTVCTILEUU4Z3P76DTWML5XANCNFSM4P2HM5GA .

aleclarson commented 2 years ago

It would be great to get this merged soon :)

jonaskello commented 2 years ago

Ok let's merge it then. I'm not using this package myself in any project anymore so I won't know if something stops working but I guess the community will post issues.

jonaskello commented 2 years ago

Ah, I did not notice there were merge conflicts. @aleclarson would you care to fix them?

aleclarson commented 2 years ago

Done #193 :)

jonaskello commented 2 years ago

Closed in #193