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

feat(dev-tools): Inject __VERSION__ when building CJS dist #439

Closed
wants to merge 1 commit into from

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Nov 3, 2023

  • We get failures in loaders.gl 4.0 when using workers from applications that bundle via CJS.
  • We can see that VERSION has not been defined in the CJS bundle.
  • Add a define statement when building CJS, same as is done by all explicit esbuild commands in loaders.gl

@ibgreen ibgreen requested a review from Pessimistress November 3, 2023 13:50
@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 3, 2023

@igorDykhta @belom88

@Pessimistress
Copy link
Collaborator

This is just an arbitrary behavior. The right solution should be using the Babel config to build the cjs bundle (where loaders adds the version-inline plugin).

Another example of cjs entry point needing special build parameters is in deck.gl where jsx must be transformed for preact.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 3, 2023

This is just an arbitrary behavior.

Version handling doesn't seem that arbitrary to me, it seems like a generic thing. We are in the business of developing frameworks and having access to version numbers is a pretty basic thing.

Whereas I agree that the preact example does indeed seem very arbitrary.

The right solution should be using the Babel config to build the cjs bundle (where loaders adds the version-inline plugin).

Hmm, I was hoping that we were aggressively moving away from babel towards tools like esbuild and perhaps later even faster tools like bun. As long as we are tied to non-native tools like babel it seems to me that our build times are likely to remain long.

But those are just my opinions. What matters is that we need to fix this asap, so unless there is a way to add the parameter through .ocularrc or similar loaders.gl will need to build its cjs dists in a different way.

@Pessimistress
Copy link
Collaborator

I was hoping that we were aggressively moving away from babel

The point is that if the source references something like __VERSION__ then it should be handled consistently between the ESM build and the bundle, controlled by either babel config or esbuild config provided by the user. Adding a special case parameter to the bundle command opens the door for more discrepancy.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 17, 2023

then it should be handled consistently between the ESM build and the bundle

Yes, that would be great. But do we have a mechanism to do something like that right now?

@ibgreen ibgreen closed this Jan 30, 2024
@Pessimistress Pessimistress deleted the ib/add-version branch February 29, 2024 03:09
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.

2 participants