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

Switch to Preconstruct for building packages #643

Closed
wants to merge 1 commit into from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented May 8, 2021

Description

The PR has been prompted by my conversation on Twitter with @jjenzz: https://twitter.com/jjenzz/status/1390973850716884994

Basically... with Preconstruct you don't need to run builds in topological order because it first just outputs dummy .d.ts that redirects requests to the respective src/index.ts. Thanks to that all builds can just happen in parallel.

@@ -17,4 +17,4 @@ plugins:
- path: .yarn/plugins/@yarnpkg/plugin-version.cjs
spec: '@yarnpkg/plugin-version'

yarnPath: .yarn/releases/yarn-2.1.1.cjs
yarnPath: .yarn/releases/yarn-2.4.1.cjs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some random reason, I couldn't install Preconstruct on a previous version because installs were failing with:

➤ YN0001: │ Error: fsevents@patch:fsevents@npm%3A2.3.2#builtin<compat/fsevents>::version=2.3.2&hash=495457: Cannot apply hunk #1
    at d (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:190674)
    at async /radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:189912
    at async h (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:188524)
    at async g (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:189885)
    at async q.fetchers.patchPackage (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:204417)
    at async B (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:226253)
    at async Q (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:226716)
    at async v (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:227192)
    at async C.fetchPackageFromCache (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:227431)
    at async q.fetchers.fetch (/radix-ui-primitives/.yarn/releases/yarn-2.1.1.cjs:2:203286)

Upgrading Yarn has helped so I didn't dig deeper into this 🤷‍♂️

@@ -15,16 +15,13 @@
"storybook": "start-storybook -p 9009 --ci",
"dev": "yarn storybook",
"build-storybook": "build-storybook",
"// build": "For context on tsconfig replacements in build scripts, see https://github.com/radix-ui/primitives/pull/361#discussion_r555004944",
"build": "yarn build:config && yarn build:packages && yarn build:cleanup",
"build:config": "mv tsconfig.json tsconfig.tmp.json && mv tsconfig.production.json tsconfig.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to the best of my knowledge - you don't need this tsconfig dance anymore

"publish": "yarn bump && yarn build && yarn workspaces foreach -pv --exclude primitives --exclude ssr-testing npm publish --tolerate-republish --access public",
"clean": "yarn workspaces foreach -pv --exclude primitives --exclude ssr-testing run clean",
"reset": "yarn clean && rm -rf node_modules .yarn/cache .parcel-cache",
"bump": "yarn version apply --all",
"bump:check": "yarn version check"
"bump:check": "yarn version check",
"_postinstall": "preconstruct dev"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current version of Yarn fails executing this postinstall with:

➤ YN0006: │ primitives@workspace:. lists build scripts, but is referenced through a soft link. Soft links don't support build scripts, so they'll be ignored.
➤ YN0007: │ primitives@workspace:. must be built because it never did before or the last one failed
➤ YN0009: │ primitives@workspace:. couldn't be built successfully (exit code 1, logs can be found here: /private/var/folders/ws/tlwxqvs5407f21nxy7w8bgr80000gp/T/xfs-e80184bd/build.log)
➤ YN0009: │ primitives@workspace:. couldn't be built successfully (exit code 1, logs can be found here: /private/var/folders/ws/tlwxqvs5407f21nxy7w8bgr80000gp/T/xfs-e80184bd/build.log)

From what I recall - at least this error message is a bug and it has already been fixed in Yarn so you could try switching to the version from their master/main branch.

"types": "dist/index.d.ts",
"main": "dist/radix-ui-number.cjs.js",
"module": "dist/radix-ui-number.esm.js",
"types": "dist/radix-ui-number.cjs.d.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically the "types" field is not required so it could be removed but there are still some reasons to keep it around. See preconstruct/preconstruct#259

@jjenzz
Copy link
Contributor

jjenzz commented May 10, 2021

Hi @Andarist,

Thanks for submitting this. It looks exactly like what we had tried already but unfortunately doesn't solve the issue. You will see after a build in your branch that Menu.d.ts contains the following:

image

😢

If it helps, we've realised this only seems to happen for our components that use our extendPrimitive utility. It uses type inference to build out the return type. I wonder if that has anything to do with it? Perhaps preconstruct doesn't know how to generate dummy .d.ts files in this case?

If we build types manually with TS in topological order, TS is able to generate the correct imports for us here.

@Andarist
Copy link
Contributor Author

@jjenzz could you describe what you are doing to get yourself into this situation on this branch? What commands do u run and what files are you inspecting?

@benoitgrelard
Copy link
Collaborator

  • Start with a clean slate, so: yarn clean
  • yarn build
  • open packages/react/menu/dist/index.d.ts
  • look for MenuAnchor

@jjenzz
Copy link
Contributor

jjenzz commented May 10, 2021

It will be in packages/react/menu/dist/declarations/src/Menu.d.ts in your preconstruct branch

@Andarist
Copy link
Contributor Author

Okay - so the problem is not as much that this doesn't work but that the generated declarations contain references to the src directory which in turn would force you to publish those, right?

I think I understand the issue better now and I agree it's a slight problem. I have an idea how this could be fixed in Preconstruct - gonna be consulting that with Mitchell and I will let u know if we can fix this for you.

@benoitgrelard
Copy link
Collaborator

That sounds great @Andarist, thanks for you time!

@jjenzz
Copy link
Contributor

jjenzz commented May 11, 2021

the generated declarations contain references to the src directory

Yep. We would expect these imports to be import("@radix-ui/rect") which we get if we build the types in topological order.

Thank you for taking a look @Andarist 🙂 In the meantime, do you mind if we close this PR until preconstruct has a way to handle it?

@Andarist
Copy link
Contributor Author

Yep. We would expect these imports to be import("@radix-ui/rect") which we get if we build the types in topological order.

I'll experiment with it more but I think there might be some cases when it's impossible to do generate import("@radix-ui/rect"). After all - a type might be internal and not exported from the root of the package. Luckily, this ain't that big of a problem given how Preconstruct generates declarations because we'll always be able to reference smth like import("@radix-ui/rect/dist/declarations/observeElementRect").

Thank you for taking a look @Andarist 🙂 In the meantime, do you mind if we close this PR until preconstruct has a way to handle it?

Sure thing - I'll close this myself now :)

@Andarist Andarist closed this May 11, 2021
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.

None yet

3 participants