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

Update to use default require.extensions #138

Closed
wants to merge 1 commit into from
Closed

Update to use default require.extensions #138

wants to merge 1 commit into from

Conversation

snuggs
Copy link

@snuggs snuggs commented Sep 22, 2017

Fixes #137, Addresses #134, & Fixes tape-testing/tape#395 by using default require.extensions collection instead of the magic Array ['.js']

Closes #166.

Fixes tape-testing/tape#396

Fixes #137 & Fixes tape-testing/tape#395 by using default `require.extensions` collection instead of the magic Array `['.js']`
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This would be a breaking change, because require.extensions defaults to have .js, .json, and .node - I'm not sure it's a particularly good idea to do in resolve.

lib/sync.js Outdated Show resolved Hide resolved
@snuggs
Copy link
Author

snuggs commented Sep 22, 2017

@ljharb to be clear this means library authors who depend on resolve would need to inform User Land to not use .json files. Otherwise authors will need to provide a flag as per our discussion in tape-testing/tape#395 (comment)

Thanks for considering this! And happy Friday! 🎉

@ljharb
Copy link
Member

ljharb commented Sep 22, 2017

Having .json files in a repo, especially fixtures in tests, is very common - I don't think that's something that will ever change in "user land".

@snuggs
Copy link
Author

snuggs commented Sep 22, 2017

Having .json files in a repo, especially fixtures in tests, is very common - I don't think that's something that will ever change in "user land". @ljharb

My point exactly. We see eye to eye.

@ljharb
Copy link
Member

ljharb commented Jan 3, 2022

@snuggs it seems you've deleted your fork, which makes this PR permanently unrecoverable, which is a shame because it'd be reasonable to land in v2.

@ljharb ljharb closed this Jan 3, 2022
@snuggs
Copy link
Author

snuggs commented Jan 3, 2022

@snuggs it seems you've deleted your fork, which makes this PR permanently unrecoverable, which is a shame because it'd be reasonable to land in v2.

Any way I can push this back up? Or has the ship left the port?

Happy New Year!

@ljharb
Copy link
Member

ljharb commented Jan 3, 2022

Unfortunately once a fork is deleted, there's no way to recover a PR, so this PR's ref will forever pollute the repo, unmergeable :-(

I think that a new PR would be reasonable, but I don't think blindly using require.extensions is a good idea yet for a few reasons:

  1. it includes .node, and shouldn't
  2. it doesn't include .cjs - but since node itself doesn't do extensions lookup for .cjs, but can resolve a full file, and we have no tests covering this, there's some work that'll need to be done first.
  3. i'm not sure if it's a good idea to include monkeypatches in it. for example, i believe the esm module adds .mjs to require.extensions, but would we want to start resolving that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants