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): allow to import modules from dist without extension #4443

Closed
wants to merge 4 commits into from

Conversation

frank-dspeed
Copy link
Contributor

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

Allow to import modules without extension from dist see: #4436 (comment)

This should address your concerns in the comment and should fix your api problem.

when there are export or import issues with that patterns simply ping me i am really deep into all this resolve stuff.

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

@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #4443 (f6c451d) into master (b8315e0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4443   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files         205      205           
  Lines        7326     7326           
  Branches     2079     2079           
=======================================
  Hits         7236     7236           
  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 b8315e0...f6c451d. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

This unfortunately breaks imports WITH extensions, so this is not an option

@frank-dspeed

This comment was marked as resolved.

@frank-dspeed

This comment was marked as resolved.

@frank-dspeed
Copy link
Contributor Author

frank-dspeed commented Mar 14, 2022

@lukastaegert list the extensions i looked into that and .d.ts is still useable as typescript resolves with its own algo and the package.json does also work i discovered no problems

you should test it if you want i can add .json .d.ts but i see no reason to do so

Rule of thumb

most specific rule gets used so you can always add more specific rules. like in css

Inspected all files via https://unpkg.com/browse/rollup@2.70.1/dist/ 
covered all cases of extensions even none needed like json and d.ts
have forgotten to allow files with the .js extension. to get imported directly
Copy link
Contributor Author

@frank-dspeed frank-dspeed left a comment

Choose a reason for hiding this comment

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

  • maps expose nested subpaths as it is a string replacement syntax only.

All instances of * on the right hand side will then be replaced with this value, including if it contains any / separators.

so * acts as ** already

The property of exports being statically enumerable is maintained with exports patterns since the individual exports for a package can be determined by treating the right hand side target pattern as a ** glob against the list of files within the package. Because node_modules paths are forbidden in exports targets, this expansion is dependent on only the files of the package itself.

replace

"./dist/**/*.js": "./dist/**/*.js", with "./dist/*.js": "./dist/*.js",

this is not a real glob pattern nodejs allows only one * per left or right hand side!

as you like docs: https://nodejs.org/api/packages.html#subpath-patterns

See review comment it is not a glob pattern only a single star per side is allowed everything gets handled as string the star is a special char it replaces instances also of / it acts as **
@frank-dspeed
Copy link
Contributor Author

frank-dspeed commented Mar 14, 2022

I need to say again so that some one who comes to this issue that the .d.ts export and the .json export should not be needed as we have only our linked types and the package.json it self will never be external imported via nodejs so it makes no sense to give it a nodejs loading rule

    "./dist/*": "./dist/*.js",
    "./dist/*.js": "./dist/*.js",

this are the only needed exports to cover all imports of all .js files with and without extension as they are string replacements with rules where the most specific gets used. so if some one writes something with .js at the end the secund gets used if he writes something without .js the first gets used.

typescript got own resolve algos that do not care for subpath patterns they are not even implemented but will get implemented and will workaround that!

but the extra .d.ts rules and .json rules are not conflicting or blocking anything so they are harmless but simply not needed.
they will simply never get triggered

also i should note that the extra package.json in the es/ folder will deactivate any rules of the ../package.json as soon as it gets a exports fild. nodejs will look it up even if you directly path import content from /es/* nodejs always will perform a tandem lookup of path/file.js path/package.json if not exist traverse till it exists or reaches /(root)

the only exempts of the tandem lookup for package.json are the *.mjs and *.cjs extensions this will never cause a additional package.json lookup as it is simply never needed the lookups do get cached. but with low ttl the cached content is always only module type and exports.

so you should never add there a exports fild when you create a lib

@lukastaegert
Copy link
Member

Did you actually try that out? Because the pattern "./dist/*.js": "./dist/*.js" does not work for me.

@guybedford
Copy link
Contributor

@lukastaegert it's supported in Node.js 14+ only, I would only recommend shipping *.js patterns once 12 goes EOL at the end of April.

@frank-dspeed
Copy link
Contributor Author

@guybedford i did not fully test it to be honest but i did read the code that implements it and it should work i am at present coding on a test app for packages so lets wait and delay the merge i will ping you once its fully working i guess first versions next week.

it will show you a simple view where the package works and what you can do to get it working as also test it with diffrent nodejs versions.

@lukastaegert
Copy link
Member

lukastaegert commented Mar 14, 2022

Actually I did not get it to work neither in Node 14.18.2 nor in Node 16.4.2 Package subpath './dist/foo.js' is not defined by "exports" in .../node_modules/test/package.json
If I remove the .js on both sides it works fine. But those are not the most recent versions of Node 14 and 16.

@guybedford
Copy link
Contributor

@frank-dspeed it looks correct to me, but as I say I would recommend avoiding until early May at least for compatibility. @lukastaegert yes it's only supported in 14.19+ and 16.9+.

@frank-dspeed
Copy link
Contributor Author

frank-dspeed commented Mar 14, 2022

@lukastaegert i would vote for droping the exports subPath Pattern filds until this can get merged as there are also cases with require without tests the most compatible way is to simply drop the subPath Patterns complet and go only with aliases import and require drop everything else this will be the most compatible way that allows at last extension less imports when require is used.

see: #4444

frank-dspeed added a commit to internet-of-presence/rollup that referenced this pull request Mar 14, 2022
chor until rollup#4443 (comment) can get merged
@guybedford
Copy link
Contributor

@frank-dspeed it is just the pattern trailers that are not supported - "./dist/*.js": "./dist/*.js" Without the extension it is supported fine back to Node.js 12 - "./dist/*": "./dist/*".

@lukastaegert
Copy link
Member

Considering all options, I would prefer to leave the logic as it is for now.

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

3 participants