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

chore(dev): Remove dev-modules/ from yarn workspace #1889

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

donmccurdy
Copy link
Collaborator

Previously babel-plugin-inline-webgl-constants was disabled because it failed inconsistently in CI. There's some tricky timing required in building the entire workspace — we ideally want to build @luma.gl/constants without the plugin, then build babel-plugin-inline-webgl-constants, then build everything else using the plugin.

Theoretically Lerna's task execution can order these steps correctly, but I believe ocular-dev-tools does not use Lerna for that, and frankly it might be trickier than expected in Lerna too. It might be best just to remove dev-modules/* from the Yarn workspace? I think this was suggested by @ibgreen in another issue or in Slack, as well.

Changes:

  • dev-modules/* should not be linked automatically, and our builds will use packages from npm unless explicitly imported from the local folder
  • dev-modules/* should not be versioned or published alongside the other packages, but can be versioned and published independently
  • dev-modules/* no longer set "private": true, which (I assume?) was a workaround to prevent the publish script from including them previously
  • Reverted the version field in each package to its last real release
  • Restored babel-plugin-inline-webgl-constants in the build script

Questions:

  • The only previously-published versions of eslint-plugin-luma-gl-custom-rules are from the v9 alpha. I'm not sure if these were intentional, or if this was intended to be a local-only package?

Alternatives:

  • Move these plugins into separate repositories
  • Have the plugins not be 'packages' but just files that we import from the relevant config (unsure if this solves anything or not...)
  • Don't strip GL constants, or don't depend on @luma.gl/constants in order to do so.

Related:

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

This has created too much confusion.

I am happy to create a new repo luma.gl-tools and move these dev-modules over there.

They really should be totally independent from the publishing process.

@@ -16,7 +16,6 @@
],
"workspaces": [
"examples/*/*",
"dev-modules/*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, this was the critical problem, I assume, that mysteriously linked everything together...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm hopeful that ocular-publish will ignore these packages now, but if not we will see errors the next time we try to do a monorepo publish, just a heads up! (Not sure how to safely test that step ahead of time...)

@@ -1,7 +1,6 @@
{
"name": "babel-plugin-inline-webgl-constants",
"private": true,
"version": "9.0.0-alpha.47",
"version": "2.0.0-alpha.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just publish 2.0.0, 2.01, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good! I've used the version strings that are already published but it could make sense to do a stable publish for each, either here or after moving into a separate repo if we prefer.

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 19, 2023

Have the plugins not be 'packages' but just files that we import from the relevant config (unsure if this solves anything or not...)

Yes... previously GL constants were part of the public API, but since that is no longer the case there is no need for a published plugin that strips constants from applications.

I do think @Pessimistress does use the shader minifier in deck.gl but it could potentially be copied there.

Overall I am pretty strongly in favor of moving away from babel (due to speed concerns) in which case we will lose these plugins anyways.

@donmccurdy donmccurdy mentioned this pull request Dec 19, 2023
31 tasks
@donmccurdy donmccurdy merged commit b4bc0fa into master Dec 19, 2023
1 check passed
@donmccurdy donmccurdy deleted the donmccurdy/no-workspace-dev-modules branch December 19, 2023 19:10
@donmccurdy donmccurdy added this to the 9.0.0 milestone Feb 26, 2024
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.

3 participants