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

chor(package.json): unlock dist in exports for programatical code reuse without patching rollup #4436

Merged
merged 1 commit into from Mar 14, 2022

Conversation

frank-dspeed
Copy link
Contributor

@frank-dspeed frank-dspeed commented Mar 10, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

This PR allows to use all files in the dist folder if they get imported directly this is needed to allow custom builds and do some code reuse.

my current usecase is a file called:

rollup.build.js

// here i need to reimplement or access the function to normalizeOptions that i get back from the rollup.config.js
// see: https://github.com/rollup/rollup/issues/4441

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #4436 (118225a) into master (511e9ae) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4436   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files         205      205           
  Lines        7324     7324           
  Branches     2080     2080           
=======================================
  Hits         7234     7234           
  Misses         33       33           
  Partials       57       57           

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 511e9ae...118225a. Read the comment docs.

@lukastaegert
Copy link
Member

lukastaegert commented Mar 10, 2022

mergeOptions is an internal API I do not feel comfortable to expose TBH. Even more so for the internal batchWarnings.
In any case, the content of shared can be subject to "API changes" even between patch versions.

@frank-dspeed
Copy link
Contributor Author

frank-dspeed commented Mar 11, 2022

@lukastaegert ok so you think its better that i replicate the rollup source in my projects, or should i always make patches for rollup versions whats your opinion. And by the way for any other engine that does not care for package.json it is already exposed even as single module file if you want to produce none reuseable code maybe consider compiling some parts to binary or even v8 snapshots.

Short version

my main reason to reuse the existing code is that i can follow api changes fast and refactor fast when i maintain extra versions which i already do! it is extra none needed overhead.

@frank-dspeed frank-dspeed changed the title Update package.json chor(package.json): unlock dist in exports for programatical code reuse without patching rollup Mar 11, 2022
@frank-dspeed
Copy link
Contributor Author

While the issue that leaded to this PR is solved i still think this should be merged it blocks people from doing what they wanna do if they use a instable API why not?

whole TS tooling is build up on none exposed API's that are even more protected There is a whole community fork of Typescript only to expose the Instable API's

coders should not block coders in a open source eco system while i understand your reasoning and it makes rollup really a good product do not get me wrong but there must be a middle way.

@lukastaegert
Copy link
Member

Fun fact: Actually everything was meant to be exposed to have compatibility between Node 10 and later versions. In the "original" version of package.exports, dist/ had one advantage over dist/*: In the first version you could require both with and without extensions and it would work, while in the second version this no longer works (if I remember correctly). Now with Node 17, dist/ has become an error. So to fix this, we need kind of a breaking change, but maybe after this long time, it is no longer relevant to many, so we could just implement it.

@lukastaegert lukastaegert merged commit 0da55f1 into rollup:master Mar 14, 2022
@frank-dspeed
Copy link
Contributor Author

@lukastaegert always a plesure to work with you

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.

None yet

2 participants