For review of #135 #139
For review of #135 #139
Conversation
Originally during the file path resolution, file extensions were removed without explicit reason. This commit changes the resolution logic to keep file extensions, with the goal to add support for modern non-js extensions like cjs. The change however keeps /xyz/index resolves as is as they still should resolve to /xyz. Refs 847d314
…e.json "main" field
cspotcode
force-pushed
the
fix-cjs-main-requires
branch
from
e28aa97
to
97f7f87
Compare
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
========================================
Coverage ? 0.00%
========================================
Files ? 4
Lines ? 129
Branches ? 52
========================================
Hits ? 0
Misses ? 129
Partials ? 0
Continue to review full report at Codecov.
|
|
Thanks for picking this up :-) LGTM |
|
@cspotcode I can add you to npm too so you can publish if you want? |
|
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 ***@***.***> 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
<#139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC35OA7HPTEVQ4NPI5MRQ3SABPRXANCNFSM4P2HM5GA>
.
|
|
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 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 :-). |
|
@cspotcode Is this ready to be merged? |
|
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
<#139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC35OCTVCTILEUU4Z3P76DTWML5XANCNFSM4P2HM5GA>
.
|
|
It would be great to get this merged soon :) |
|
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. |
|
Ah, I did not notice there were merge conflicts. @aleclarson would you care to fix them? |
|
Done #193 :) |
|
Closed in #193 |
This PR is meant to be #135, but I had to push to a different branch.