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

fix(dev_env_setup): use relative packages everywhere #8910

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johancube
Copy link
Contributor

@johancube johancube commented Nov 4, 2024

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made (if issue reference is not provided)

This solution ensures that all related packages work together seamlessly without concern for missing links.

@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Nov 4, 2024
Copy link
Member

@KSDaemon KSDaemon left a comment

Choose a reason for hiding this comment

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

👍🏻 Great improvement and simplification! But I think there are some things to be resolved. Left comments inline.

@@ -12,145 +12,75 @@ cd "$SCRIPT_DIR"
echo "Running 'yarn install' in the root directory..."
yarn install

# Step 2: Run yarn build in the root directory
echo "Running 'yarn build' in the root directory..."
yarn build
Copy link
Member

Choose a reason for hiding this comment

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

originally yarn build fires the rollup to build all the UMDs/ESMs in packages... You switched to trying running yarn build inside each pkg. But does this produce the same output?
Taking package packages/cubejs-client-core as an example:
There is no build script in package.json and there is no ts files. So new version won't do anything, while original rollup will prepare UMD for it.

do
echo "Linking @cubejs-backend/$driver in packages/cubejs-server-core..."
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure linking drivers in server-core is no longer needed for proper working?

if [ -d "$package" ]; then
package_name=$(node -p "require('$package/package.json').name")
echo "Linking $package_name..."
yarn link "$package_name"
Copy link
Member

Choose a reason for hiding this comment

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

It's probably good to cleanup the scripts in root package.json to remove potential confusion for those who will find it and try to use but it is outdated and not actual anymore. Like "link:dev"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants