FormidableLabs / spectacle

A React-based library for creating sleek presentations using JSX syntax that gives you the ability to live demo your code.
https://commerce.nearform.com/open-source/spectacle/
MIT License
9.75k stars 691 forks source link

Chore/adding types check #1216

Closed paulmarsicloud closed 2 years ago

paulmarsicloud commented 2 years ago

Description

Changing typecheck in website to types:check Removing typescript dev dependency from website Adding shamefully-hoist=true to .npmrc in order for types:check to work from root. If shamefully-hoist=false (default setting here) we we're getting this error

Fixes #1203

Type of Change

Please delete options that are not relevant (including this descriptive text).

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Checklist: (Feel free to delete this section upon completion)

gksander commented 2 years ago

@paulmarsicloud The whole shamefully-hoist=true thing makes me feel shame 😆 I just pulled this branch down, removed that line, nuked cache and re-installed stuff – and was able to run pnpm run check:ci and get everything passing on the first try.

Do you have any more details on how to reproduce the error you encountered that you needed the hoisting for? I feel like I'm probably missing something very obvious.

ryan-roemer commented 2 years ago

Let me try and experiment without shamefully-hoist=true and see what I can find.

ryan-roemer commented 2 years ago

@paulmarsicloud Here's a much more surgical hoisting allowance for the bad phantom dependency:

diff --git a/.npmrc b/.npmrc
index c67cd2d..519f90d 100644
--- a/.npmrc
+++ b/.npmrc
@@ -1,3 +1,5 @@
 strict-peer-dependencies=false
 prefer-workspace-packages=true
-shamefully-hoist=true
\ No newline at end of file
+
+# Docusaurus has some phantom dependencies, so specifically hoist those.
+public-hoist-pattern[]=@docusaurus/theme-classic
diff --git a/website/package.json b/website/package.json
index 81a1b08..926a5e4 100644
--- a/website/package.json
+++ b/website/package.json
@@ -12,8 +12,10 @@
     "@docusaurus/preset-classic": "^2.0.1",
     "@docusaurus/theme-live-codeblock": "^2.0.1",
     "@docusaurus/theme-search-algolia": "^2.0.1",
+    "@docusaurus/types": "^2.0.1",
     "@mdx-js/react": "^1.6.22",
     "@tsconfig/docusaurus": "^1.0.5",
+    "@types/node": "^18.0.3",
     "@types/styled-components": "^5.1.25",
     "clsx": "^1.1.1",
     "docusaurus-plugin-sass": "^0.2.2",

Want me to make a commit (alongside the pnpm-lock.yaml changes) and push up?

paulmarsicloud commented 2 years ago

@paulmarsicloud Here's a much more surgical hoisting allowance for the bad phantom dependency:

diff --git a/.npmrc b/.npmrc
index c67cd2d..519f90d 100644
--- a/.npmrc
+++ b/.npmrc
@@ -1,3 +1,5 @@
 strict-peer-dependencies=false
 prefer-workspace-packages=true
-shamefully-hoist=true
\ No newline at end of file
+
+# Docusaurus has some phantom dependencies, so specifically hoist those.
+public-hoist-pattern[]=@docusaurus/theme-classic
diff --git a/website/package.json b/website/package.json
index 81a1b08..926a5e4 100644
--- a/website/package.json
+++ b/website/package.json
@@ -12,8 +12,10 @@
     "@docusaurus/preset-classic": "^2.0.1",
     "@docusaurus/theme-live-codeblock": "^2.0.1",
     "@docusaurus/theme-search-algolia": "^2.0.1",
+    "@docusaurus/types": "^2.0.1",
     "@mdx-js/react": "^1.6.22",
     "@tsconfig/docusaurus": "^1.0.5",
+    "@types/node": "^18.0.3",
     "@types/styled-components": "^5.1.25",
     "clsx": "^1.1.1",
     "docusaurus-plugin-sass": "^0.2.2",

Want me to make a commit (alongside the pnpm-lock.yaml changes) and push up?

That makes sense, go for it!

gksander commented 2 years ago

@paulmarsicloud wanted to check in on this – how do you feel about merging this?

paulmarsicloud commented 2 years ago

@paulmarsicloud wanted to check in on this – how do you feel about merging this?

works for me! we ok with 1 approval or shall I seek a second?

gksander commented 2 years ago

I think 1 will be alright for this!