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 exports to package.json to restrict access to private internals #2208

Merged
merged 1 commit into from Nov 23, 2021

Conversation

bmish
Copy link
Member

@bmish bmish commented Nov 19, 2021

https://nodejs.org/api/packages.html#exports

This prevents users from importing files directly from this package.

ESLint v8 also added this restriction.

Beginning in v8.0.0, ESLint is strictly defining its public API. Previously, you could reach into individual files such as require("eslint/lib/rules/semi") and this is no longer allowed. There are a limited number of existing APIs that are now available through the /use-at-your-own-risk entrypoint for backwards compatibility, but these APIs are not formally supported and may break or disappear at any point in time.

Ideally, we'll hear from users after making this change if there are additional internals that we need to expose as part of our public NodeJS API in lib/index.js.

Part of v4 release (#1908).

TODO:

@bmish bmish added the breaking label Nov 19, 2021
@bmish bmish mentioned this pull request Nov 19, 2021
58 tasks
@bmish bmish requested a review from rwjblue November 19, 2021 04:56
@@ -19,7 +19,8 @@
},
"license": "MIT",
"author": "Robert Jackson <me@rwjblue.com>",
"main": "lib/index.js",
"exports": "./lib/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

I think we will need to expose lib/helpers/rule-test-harness.js as well. It is used by the plugins to test their rules (example).

But maybe it is the good time to expose it through lib/index.js? Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, my preference would be that we expose that through lib/index.js, as I've implemented here: #2209

@bmish
Copy link
Member Author

bmish commented Nov 19, 2021

I added to the description of this PR that there's another item we need to solve:

What to do about users who may need to retrieve the default config from no-bare-strings? const { DEFAULT_CONFIG } = require('ember-template-lint/lib/rules/no-bare-strings');

@bmish bmish requested a review from a team November 19, 2021 19:36
@bmish bmish added this to the 4.0.0 milestone Nov 19, 2021
@dcyriller
Copy link
Member

dcyriller commented Nov 19, 2021

What to do about users who may need to retrieve the default config from no-bare-strings? const { DEFAULT_CONFIG } = require('ember-template-lint/lib/rules/no-bare-strings');

It is difficult to tell if the default config from no-bare-strings is really consummed by some ember-template-lint plugins. TBH I have no idea.

I see two options:

  • we don't expose it and wait for users to report an issue if they need it;
  • we expose it through the lib/index.js file. This option has the advantage of being more gentle (no bug, no issue to report) but the drawback of increasing the cost of maintenance of ember-template-lint (# of files exposed, etc.)

I see your PR as the opportunity to keep the API surface under control. Then, I would not expose the default config from no-bare-strings. But maybe there are other opinions.

@bmish
Copy link
Member Author

bmish commented Nov 19, 2021

@dcyriller thanks for the feedback. I agree we should avoid exposing things unnecessarily.

I'm going to redesign the no-bare-strings rule configuration a bit so that there's no need for consumers to import the default config from the rule. The new behavior of the rule options will be to augment instead of replace the default config, as augmenting is typically what consumers want to do. If anyone really needs to replace the default config, then we can add boolean options to the rule ignoreDefaultAllowList, ignoreDefaultGlobalAttributes, ignoreDefaultElementAttributes (default false) to get the old behavior back.

@dcyriller
Copy link
Member

I see, thanks for the explanation. I gave it a try in the linked PR.

@bmish bmish merged commit c7167c1 into ember-template-lint:next Nov 23, 2021
@bmish bmish deleted the exports-package-json branch November 23, 2021 18:42
@bmish bmish changed the title Add exports to package.json Add exports to package.json to restrict access to private internals Nov 24, 2021
@dcyriller
Copy link
Member

FYI I noticed ember-template-lint-plugin-tailwind uses lib/helpers/create-error-message.

@bmish
Copy link
Member Author

bmish commented Dec 14, 2021

@dcyriller good catch. I don't really like this clunky createErrorMessage function. ESLint definitely doesn't have an equivalent. I'm hoping we can replace this with a future schema API (#1371) or other built-in functionality.

I'm leaning toward not publishing createErrorMessage unless it's heavily used by the plugin ecosystem. If we discover that lots of plugins need it, we can add it to the public API until we have a better alternative.

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

Successfully merging this pull request may close these issues.

None yet

2 participants