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(spectral): esm again #746

Merged
merged 11 commits into from
Sep 8, 2023
Merged

feat(spectral): esm again #746

merged 11 commits into from
Sep 8, 2023

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Sep 8, 2023

🧰 Changes

We were running into issues with alex not being properly bundled but thanks to tsup we're now including it in the dist. This SHOULD work now :cry-party:

(Tagging #744 #724 #718 for posterity)

🧬 QA & Testing

I've been QA'ing by linking the package manually with the following Spectral config in a separate repository:

extends:
  - ../standards/packages/spectral-config

Instead of the reckless non-QA approach I took in previous PRs, I actually feel good about this QA process translating to actual experience using the package.

@@ -2,12 +2,16 @@
"name": "@readme/spectral-config",
"version": "3.0.6",
"description": "ReadMe coding standards for API documentation",
"main": "src/index.js",
"main": "dist/index.cjs",
Copy link
Member

Choose a reason for hiding this comment

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

do you need the exports: {} stuff here?

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think so, i've been QA'ing using the readme-metrics-api main repo it works with this spectral config:

CleanShot 2023-09-08 at 14 28 34@2x

"lint": "eslint .",
"prepack": "npm run build",
Copy link
Member Author

Choose a reason for hiding this comment

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

@erunion is this the only thing needed for lerna to build the dist before publishing?

Copy link
Member

Choose a reason for hiding this comment

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

might add a prebuild for blowing away the dist directory too. i dont know if the tsup clean directory will do that or not.

"prebuild": "rm -rf dist/"

Copy link
Member Author

@kanadgupta kanadgupta Sep 8, 2023

Choose a reason for hiding this comment

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

i think we should be okay on that front b/c clean: true in the tsup config

edit: sorry just saw the second part of your comment lol i confirmed it cleans it out properly!

@kanadgupta kanadgupta marked this pull request as ready for review September 8, 2023 19:41
@kanadgupta kanadgupta merged commit bb437f8 into main Sep 8, 2023
3 checks passed
@kanadgupta kanadgupta deleted the spectral/esm-tsup branch September 8, 2023 19:42
kanadgupta added a commit that referenced this pull request Sep 8, 2023
erunion pushed a commit that referenced this pull request Sep 8, 2023
kanadgupta added a commit that referenced this pull request Sep 8, 2023
erunion pushed a commit that referenced this pull request Sep 8, 2023
* Revert "Revert "feat(spectral): esm again (#746)" (#748)"

This reverts commit b0bd700.

* fix: point the `main` field to the ESM export
@kanadgupta kanadgupta added enhancement New feature or request dependencies Pull requests that update a dependency file labels Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants