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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs/clean: formally deprecate rollupCommonJSResolveHack #367

Merged
merged 2 commits into from Jun 24, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jun 23, 2022

Summary

Formally deprecate rollupCommonJSResolveHack as it's been obsolete since 0.30.0

Details

  • this has had no effect since Fix duplicate output with multiple entry points on Windows (host normalization)聽#251

    • that changed the code to always return OS native paths via the NodeJS Path API
      • so setting rollupCommonJSResolveHack would make no difference either true or false
        • effectively, it's as if it's always true now
  • formally state now that this is deprecated in the docs

    • as well as when that occurred and what it means
  • also add a warning in options similar to the existing one for objectHashIgnoreUnknownHack (from cd76b42, which was added to my first major contribution back in (fix): upgrade object-hash to support async/await syntax聽#203 馃檪 )

  • remove the resolve dependency as well

    • it turns out something in the devDeps still uses it, so it didn't get fully removed in the package-lock.json
    • resolve was never needed anyway as we could've used NodeJS's native path.resolve or require.resolve instead (noted this at the bottom of rpt2 does not resolve pnpm symlinked dependencies聽#234 (comment))
      • resolve was created for browserify after all, where one can't use NodeJS APIs
        • but we run on NodeJS and can and already do use NodeJS APIs, including both path.resolve and require.resolve
    • I actually started this PR just to remove the dep, then realized the entire code path is obsolete 馃槄 馃槄

References

- this has had no effect since 6fb0e75, released in 0.30.0
  - that changed the code to always return OS native paths via the NodeJS Path API
    - so setting `rollupCommonJSResolveHack` would make no difference either `true` or `false`
      - effectively, it's as if it's always `true` now

- formally state now that this is deprecated in the docs
  - as well as when that occurred and what it means

- also add a warning in `options` similar to the existing one for `objectHashIgnoreUnknownHack`

- remove the `resolve` dependency as well
  - it turns out something in the devDeps still uses it, so it didn't get fully removed in the `package-lock.json`
  - `resolve` was never needed anyway as we could've used NodeJS's native `path.resolve` or `require.resolve` instead
    - `resolve` was created for `browserify` after all, where one can't use NodeJS APIs
      - but we run on NodeJS and can and already do use NodeJS APIs, including both `path.resolve` _and_ `require.resolve`
  - I actually started this commit just to remove the dep, then realized the entire code path is obsolete
@agilgur5 agilgur5 added scope: docs Documentation could be improved. Or changes that only affect docs topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: integration Related to an integration, not necessarily to core (but could influence core) labels Jun 23, 2022
@ezolenko ezolenko merged commit 74f6761 into ezolenko:master Jun 24, 2022
@agilgur5 agilgur5 added kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc and removed kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs labels Jul 22, 2022
@agilgur5 agilgur5 deleted the docs-clean-deprecate-commonjs-hack branch July 2, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc scope: docs Documentation could be improved. Or changes that only affect docs scope: integration Related to an integration, not necessarily to core (but could influence core) topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants