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

feat!: block access to /lib #14352

Merged
merged 1 commit into from Apr 9, 2022
Merged

feat!: block access to /lib #14352

merged 1 commit into from Apr 9, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Apr 9, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

This PR prevents users from importing anything other than @sequelize/core and @sequelize/core/package.json.

With this change, it is safe to rework our internal source code without risking to introduce a breaking change.

If users need to access our internals, they should open a feature request so we can expose a properly tested solution.

Breaking change documented: sequelize/website#78

As a last resort, if a user really needs to access them, I added @sequelize/core/_non-semver-use-at-your-own-risk_ as an alias for @sequelize/core/lib. Hopefully it is clear enough that these imports should only be used as a last resort and are not officially supported, and could break in any release, including patch releases.

@WikiRik
Copy link
Member

WikiRik commented Apr 9, 2022

Why would a user want to import our package.json?

@ephys
Copy link
Member Author

ephys commented Apr 9, 2022

Typically to access our version number (even though it's exposed as Sequelize.vesrion), like in this issue #13787. Other packages also expose their package.json even though their other modules are restricted, so there may be reasons I don't know about

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Can't hurt to export package.json then, so should be good

@ephys ephys merged commit f1774ec into main Apr 9, 2022
@ephys ephys deleted the ephys/no-sequelize-internals branch April 9, 2022 15:52
@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0-alpha.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

vanthome pushed a commit to vanthome/sequelize that referenced this pull request Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants