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

add --preserve-extensions option #8

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WickyNilliams
Copy link

@WickyNilliams WickyNilliams commented Mar 22, 2024

  • added option --present-extensions which keeps .js extensions
    • handles exports
    • handles typesVersions
    • handles wrappers
  • add test
  • fixed up some jsdoc types which were incorrect
  • small typo fix "nain" -> "main"

const mappedExports = {};
const mappedTypes = {};

for (const key in result.pkg.exports) {
Copy link
Author

Choose a reason for hiding this comment

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

i used for..in here because i was getting inconsistent ordering between test runs when using Object.entries and Object.fromEntries and i think this should preserve order

Copy link
Author

@WickyNilliams WickyNilliams Mar 22, 2024

Choose a reason for hiding this comment

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

actually, i am still getting inconsistent results. it fails about 50% of the time. there must be some kind of race condition or shared state between test runs 🤔

it's always the same failure when it happens, these two exports entries end up swapped around. but which test fails also varies. very strange

  -         "./components/a.js": {
  -             "default": "./tests/module/components/a/a.js"
  +         "./components/b.js": {
  +             "default": "./tests/module/components/b/b.js"
            },
  -         "./components/b.js": {
  -             "default": "./tests/module/components/b/b.js"
  +         "./components/a.js": {
  +             "default": "./tests/module/components/a/a.js"

Copy link
Author

@WickyNilliams WickyNilliams Mar 22, 2024

Choose a reason for hiding this comment

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

even running the tests in serial (-s flag in ava) i get this issue. not sure what the cause is

@WickyNilliams WickyNilliams force-pushed the feature/preserve-export-extensions branch from 572cf5d to 593ae18 Compare March 22, 2024 13:40
@WickyNilliams WickyNilliams force-pushed the feature/preserve-export-extensions branch from 593ae18 to d358bbe Compare March 22, 2024 13:41
src/merge-exports.js Outdated Show resolved Hide resolved
@WickyNilliams
Copy link
Author

WickyNilliams commented Mar 22, 2024

I wonder if there is an opportunity to simplify typesVersions by following this kind of wildcard approach described here or here, so that you don't need to list every type location.

I don't know much about how typesVersions works, so perhaps there isn't. But thought i would ask!

@WickyNilliams WickyNilliams marked this pull request as draft March 22, 2024 14:49
@UpperCod
Copy link
Member

Hi @WickyNilliams , great PR! I have refactored the workspaces for testing, deprecating the use of ava and switching to node:test. Now it offers the following benefits:

  1. Matching based on specific files.
  2. Each test is a package, so you can associate specific dependencies for the test, such as TypeScript + @atomico/exports.

Now, upon reviewing the output, I noticed this type of output:

If the index is equal to ".js", we should transform it to "."

@UpperCod UpperCod closed this Mar 23, 2024
@UpperCod UpperCod reopened this Mar 23, 2024
@UpperCod
Copy link
Member

I wonder if there is an opportunity to simplify typesVersions by following this kind of wildcard approach described here or here, so that you don't need to list every type location.

I don't know much about how typesVersions works, so perhaps there isn't. But thought i would ask!

The adoption of package.json#exports makes me wonder if it's time to deprecate typesVersions.

(By the way, I didn't intend to close the PR; it was a clicking error while typing on my phone.)

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.

2 participants