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 babel-plugin-inline-webgl-constants #1859

Merged

Conversation

donmccurdy
Copy link
Collaborator

Fixes for babel-plugin-inline-webgl-constants in v9. I haven't published the plugin to npm, so I haven't added it back to babel.config.cjs yet either. When building locally, I needed to disable the ocular-clean step before each build, so that the constants are there when the plugin imports them. I don't know if that would be an issue once the plugin is published to npm? Or should we be doing something to ensure @luma.gl/constants builds first, without this plugin?

Changes:

  • Handle both default and non-default imports
  • Skip certain expressions so that including the babel plugin when building @luma.gl/constants does not fail
  • Update and re-enable unit tests

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 2, 2023

If I recall correctly, in this module we used to work around the "inliner" simply by importing the modules under a different name `import {GL as GLEnum, GL as GL_CONSTANTS, ...}.

The webgl-types.h file is new for v9 so it hasn't been done there (as the inliner has been disabled). Maybe a rename is all that is needed?

The main other thing that changed in the constants module for v9 is that we had to change from GL being a default export to a named export, as default exports from modules have issues when using ES modules (package type "module").

@donmccurdy
Copy link
Collaborator Author

I'll take a closer look on Monday, but I think renaming GL as GLEnum would be an alternative to how I'm using EXCLUDED_TYPES here. I'd be fine with either of those solutions. This PR does handle the "default export to a named export" change.

I'm still not quite sure how the babel plugin can import @luma.gl/constants before @luma.gl/constants is built, and since it's a part of that build process, it seems like a circular dependency? But if that hasn't been an issue in the past, maybe it isn't a real problem.

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 2, 2023

Note that the dev-modules folder is not part of the synchronized publishing process that we use for the main packages in the modules directory. You publish the dev modules separately, individually, "manually" at any time.

@donmccurdy
Copy link
Collaborator Author

I think I've resolved the open issues. Using a "GLEnum" alias instead of "EXCLUDED_TYPES" feels like a good approach to me. The rest of the changes are required to support the ESM import syntax.

For future reference, my steps to make changes in this plugin were:

  1. Run yarn build with the plugin disabled
  2. Disable ocular-clean in package.json#scripts build macro
  3. Enable plugin './dev-modules/babel-plugin-inline-webgl-constants/index.js' in babel.config.js
  4. Run yarn && yarn build from dev-modules/babel-plugin-inline-webgl-constants
  5. Run yarn build and/or yarn test from project root

The unit tests are especially helpful in this case!

@donmccurdy
Copy link
Collaborator Author

Someone else may need to publish babel-plugin-inline-webgl-constants to npm for me, before this can be enabled in babel.config.js. For now I've made the changes but not enabled it in babel config.

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.

LGTM, Let's get a stamp from @Pessimistress as well.

@ibgreen ibgreen requested a review from Pessimistress December 5, 2023 17:46
@donmccurdy
Copy link
Collaborator Author

Thanks! I'll publish the plugin and merge the PR tomorrow.

@donmccurdy donmccurdy merged commit 3ab06ea into master Dec 6, 2023
1 check passed
@donmccurdy donmccurdy deleted the donmccurdy/v9-babel-plugin-inline-webgl-constants branch December 6, 2023 14:40
@donmccurdy
Copy link
Collaborator Author

Seeing that the plugin's versioning is synced with the other luma.gl packages, I'm not confident publishing a release of the whole monorepo myself yet. I'll follow up about that in Slack.

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