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

Don't enforce module to precede import per se #50

Merged
merged 2 commits into from
Jul 5, 2023
Merged

Conversation

nvie
Copy link
Contributor

@nvie nvie commented Jul 5, 2023

The lint rule I contributed recently in #49 may have been a tad bit too strict. Some package authors prefer to use the module condition to override the require (or default) condition only, but not using it for import statements. That should be fine. The purpose of the bundler-specific module condition is to allow you to override/intercept require()s mostly, and instruct the bundler to include the ESM version instead.

As a package author, you can still also decide to place the module condition before the import and as such use it to also intercept import statements. Both approaches are fine, as a module and import definition will typically resolve to the same module in practice. So it's mostly an author preference, and not something we should enforce with a lint rule.

This PR loosens the check by only enforcing that module precedes require. It drops the requirement that it also precedes import.

An example of a project that got a false positive from the too-strict check is https://publint.dev/zustand. They use import, and only fall back to a module redirect for require() statements (via their default condition). That's perfectly fine.


For example, examples 1 and 2 are fine, but example 3 is not (the "module" will never be used):

Example 1:

{
  ".": {
    "module": "index.mjs",  // 👍 Still fine
    "import": "index.mjs",
    "require": "index.cjs",
  }
}

Example 2:

{
  ".": {
    "import": "index.mjs",
    "module": "index.mjs",  // 👍 Previously rejected, now also fine
    "require": "index.cjs",
  }
}

Example 3:

{
  ".": {
    "import": "index.mjs",
    "require": "index.cjs",
    "module": "index.mjs",  // ❌ Lint error, because this condition will never be used
  }
}

nvie added 2 commits July 5, 2023 09:24
There are still valid set ups that define "module" below "import". What
matters is that it still precedes "require", though.

For example, examples 1 and 2 are fine, but example 3 is not (the
"module" will never be used):

Example 1:
{
  ".": {
    "module": "index.mjs",  // 👍
    "import": "index.mjs",
    "require": "index.cjs",
  }
}

Example 2:
{
  ".": {
    "import": "index.mjs",
    "module": "index.mjs",  // 👍
    "require": "index.cjs",
  }
}

Example 3:
{
  ".": {
    "import": "index.mjs",
    "require": "index.cjs",
    "module": "index.mjs",  // ❌ Never used
  }
}
Copy link
Owner

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Sounds fair to me. Trying to think if removing a message is considered a breaking change, but it's probably fine. Will cut this as a patch later.

Thanks again for keeping up with this!

@bluwy bluwy merged commit 9d18c31 into bluwy:master Jul 5, 2023
@nvie
Copy link
Contributor Author

nvie commented Jul 5, 2023

Trying to think if removing a message is considered a breaking change, but it's probably fine.

I think it's fine too.

Will cut this as a patch later.

Thanks for the merge, and for this invaluable tool for package authors! 🙏

tnez referenced this pull request in tnez/actions Jul 10, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [publint](https://publint.dev)
([source](https://togithub.com/bluwy/publint)) | [`0.1.15` ->
`0.1.16`](https://renovatebot.com/diffs/npm/publint/0.1.15/0.1.16) |
[![age](https://badges.renovateapi.com/packages/npm/publint/0.1.16/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/publint/0.1.16/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/publint/0.1.16/compatibility-slim/0.1.15)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/publint/0.1.16/confidence-slim/0.1.15)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>bluwy/publint (publint)</summary>

### [`v0.1.16`](https://togithub.com/bluwy/publint/releases/tag/v0.1.16)

[Compare
Source](https://togithub.com/bluwy/publint/compare/v0.1.15...v0.1.16)

##### Bug fixes

- Don't enforce the `module` condition to precede `import` per se. It is
now ensured to precede `require` only as otherwise the condition isn't
effective
([https://github.com/bluwy/publint/pull/50](https://togithub.com/bluwy/publint/pull/50))

**Full Changelog**:
bluwy/publint@v0.1.15...v0.1.16

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/tnez/actions).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNDQuMiIsInVwZGF0ZWRJblZlciI6IjM1LjE0NC4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
kodiakhq bot referenced this pull request in ascorbic/unpic-img Sep 3, 2023
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [publint](https://publint.dev) ([source](https://togithub.com/bluwy/publint)) | [`^0.1.12` -> `^0.2.0`](https://renovatebot.com/diffs/npm/publint/0.1.12/0.2.2) | [![age](https://developer.mend.io/api/mc/badges/age/npm/publint/0.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/publint/0.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/publint/0.1.12/0.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/publint/0.1.12/0.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>bluwy/publint (publint)</summary>

### [`v0.2.2`](https://togithub.com/bluwy/publint/releases/tag/v0.2.2)

[Compare Source](https://togithub.com/bluwy/publint/compare/v0.2.1...v0.2.2)

##### Features

-   Lint `"typings"` field file existence ([https://github.com/bluwy/publint/pull/60](https://togithub.com/bluwy/publint/pull/60))
-   Check packed files when globbing exports locally ([https://github.com/bluwy/publint/issues/61](https://togithub.com/bluwy/publint/issues/61))
-   Improve `"browser"` field suggestion for using `"imports"` and `"exports"` fields instead ([https://github.com/bluwy/publint/pull/59](https://togithub.com/bluwy/publint/pull/59))

##### Bug fixes

-   Lower deprecated trailing slash glob syntax as `suggestion` instead of a `warning` when it's used for backwards compatibility only ([https://github.com/bluwy/publint/issues/62](https://togithub.com/bluwy/publint/issues/62))
-   Suppress invalid globbed file format if has correct adjacent file
-   Fix extension replacement in messages
-   Improve invalid types format message and docs

##### Site

-   Fix invalid package name not found message
-   Highlight code blocks in rules page

##### New Contributors

-   [@&#8203;btea](https://togithub.com/btea) made their first contribution in [https://github.com/bluwy/publint/pull/64](https://togithub.com/bluwy/publint/pull/64)

**Full Changelog**: bluwy/publint@v0.2.1...v0.2.2

### [`v0.2.1`](https://togithub.com/bluwy/publint/releases/tag/v0.2.1)

[Compare Source](https://togithub.com/bluwy/publint/compare/v0.2.0...v0.2.1)

##### Bug fixes

-   Fix `"types"` condition check with `"exports"` array format
-   Disable packed files search when a `vfs` is passed
-   Fix `"browser"` field file existence extensions check
-   Fix file existence check with trailing slash

##### Site

-   Site-wide design touch-up
-   New "Popular packages" section
-   New package version select switcher ([https://github.com/bluwy/publint/issues/56](https://togithub.com/bluwy/publint/issues/56))
-   New navigation header design
-   Update bottom documentation for clarity
-   Improve repo URL parsing

##### New Contributors

-   [@&#8203;lachlancollins](https://togithub.com/lachlancollins) made their first contribution in [https://github.com/bluwy/publint/pull/53](https://togithub.com/bluwy/publint/pull/53)

**Full Changelog**: bluwy/publint@v0.2.0...v0.2.1

### [`v0.2.0`](https://togithub.com/bluwy/publint/releases/tag/v0.2.0)

[Compare Source](https://togithub.com/bluwy/publint/compare/v0.1.16...v0.2.0)

##### Breaking changes

**Note:** If you're using `publint` from the CLI, these breaking changes should not affect you.

-   `publint()` now returns an object with `messages` instead of the `messages` array directly. This makes way for future APIs where `publint` will return more information than just `messages`.

    ```diff
    - const messages = await publint()
    + const { messages } = await publint()
    ```

-   Rename `printMessage` API to `formatMessage` to better reflect it's intent. ([https://github.com/bluwy/publint/issues/43](https://togithub.com/bluwy/publint/issues/43))

    ```diff
    - import { printMessage } from "publint/utils"
    + import { formatMessage } from "publint/utils"

    const { messages } = await publint()

    for (const message of messages) {
    - console.log(printMessage(message))
    + console.log(formatMessage(message))
    }
    ```

-   Remove `filePath` `arg` for the `FILE_DOES_NOT_EXIST` message.

    ```diff
    import type { Message } from "publint"
    import { getPkgPathValue } from "publint/utils"

    function messageToString(message: Message, pkg: Record<string, any>) {
      switch (message.code) {
        case "FILE_DOES_NOT_EXIST":
    -     return `The file "${message.args.filePath}" does not exist.`
    +     return `The file "${getPkgPathValue(pkg, message.path)}" does not exist.`
      }
    }
    ```

-   Remove the `import` condition for the `publint` package. This provides a better error message if you call `require("publint")`.

##### Features

-   Improve warnings when the exported `"types"` condition has an invalid format in ESM or CJS. This ensures your library's types will work in both environments when dual publishing. ([https://github.com/bluwy/publint/issues/46](https://togithub.com/bluwy/publint/issues/46))

    It affects packages commonly packaged like:

    ```json
    {
      "exports": {
        ".": {
          "types": "./index.d.ts", <-- only works in CJS
          "import": "./index.mjs",
          "require": "./index.js",
        }
      }
    }
    ```

    For more information, visit the [rules documentation](https://publint.dev/rules#export_types_invalid_format). This feature is inspired by https://arethetypeswrong.github.io.

##### Bug fixes

-   Suppress warnings when exported JS files using the `"exports"` field have adjacent `.d.ts` files and no `"types"` condition. This follows TypeScript's resolution algorithm. For more information, visit the [rules documentation](https://publint.dev/rules#types_not_exported). ([https://github.com/bluwy/publint/issues/46](https://togithub.com/bluwy/publint/issues/46))

**Full Changelog**: bluwy/publint@v0.1.16...v0.2.0

### [`v0.1.16`](https://togithub.com/bluwy/publint/releases/tag/v0.1.16)

[Compare Source](https://togithub.com/bluwy/publint/compare/v0.1.15...v0.1.16)

##### Bug fixes

-   Don't enforce the `module` condition to precede `import` per se. It is now ensured to precede `require` only as otherwise the condition isn't effective ([https://github.com/bluwy/publint/pull/50](https://togithub.com/bluwy/publint/pull/50))

**Full Changelog**: bluwy/publint@v0.1.15...v0.1.16

### [`v0.1.15`](https://togithub.com/bluwy/publint/releases/tag/v0.1.15)

[Compare Source](https://togithub.com/bluwy/publint/compare/v0.1.14...v0.1.15)

##### Bug fixes

-   Fix "precede" typo

**Full Changelog**: bluwy/publint@v0.1.14...v0.1.15

### [`v0.1.14`](https://togithub.com/bluwy/publint/releases/tag/v0.1.14)

[Compare Source](https://togithub.com/bluwy/publint/compare/v0.1.13...v0.1.14)

##### Features

-   Check that the `"module"` condition precedes the `"import"` and `"require"` in exports conditions ([https://github.com/bluwy/publint/pull/49](https://togithub.com/bluwy/publint/pull/49))

##### Bug fixes

-   Skip linting flow files
-   Improve exports array logging format

##### Site

-   Fix `isPathDir` check

##### New Contributors

-   [@&#8203;nvie](https://togithub.com/nvie) made their first contribution in [https://github.com/bluwy/publint/pull/49](https://togithub.com/bluwy/publint/pull/49)

**Full Changelog**: bluwy/publint@v0.1.13...v0.1.14

### [`v0.1.13`](https://togithub.com/bluwy/publint/releases/tag/v0.1.13)

[Compare Source](https://togithub.com/bluwy/publint/compare/v0.1.12...v0.1.13)

##### Bug fixes

-   Fix `"types"` condition-is-first check when there's preceding conditions that has it's `"types"` condition too. This is common for dual ESM-CJS packages where `"types"` are located within `"require"` and `"import"` conditions. ([https://github.com/bluwy/publint/issues/47](https://togithub.com/bluwy/publint/issues/47))
-   Temporarily skip `"types"` condition check when `"typesVersions"` key exist. The `"typesVersions"` key requires a complex resolution algorithm that is harder to implement, so a quick patch is applied to remove the false errors for now. ([https://github.com/bluwy/publint/issues/42](https://togithub.com/bluwy/publint/issues/42))

**Full Changelog**: bluwy/publint@v0.1.12...v0.1.13

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 9pm on sunday" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/ascorbic/unpic-img).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi43OC44IiwidXBkYXRlZEluVmVyIjoiMzYuNzguOCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
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