Kitware / vtk-js

Visualization Toolkit for the Web
https://kitware.github.io/vtk-js/
BSD 3-Clause "New" or "Revised" License
1.23k stars 371 forks source link

Consider alternative mechanism for vtk*Macro #108

Closed zachmullen closed 7 years ago

zachmullen commented 7 years ago

The status quo is that these symbols (e.g. vtkErrorMacro) are string-replaced via a loader that runs before the babel loader during the build process, which was probably done to mimic the preprocessor behavior in C++. This seems unnecessary, though, since we can bind or configure those methods at runtime easily and skip the global find/replace of strings. This forces downstream users wishing to build vtkjs with webpack to replicate these loader rules in their own configurations.

Generally speaking, I think in a javascript environment, we should probably try to avoid mimicking the behavior of the C++ preprocessor since it's counterintuitive to JS developers and breaks with the normal mechanics of the module system.

jourdain commented 7 years ago

It was not necessary to mimic C++ but to ease the migration of the C++ code while keeping the distinction between debug, warn, error... That seems to be a quick and easy way to do it at that time. Also having a pre-processor in hand was giving us option to patch things before we fix them properly after some thoughts.

But I agree, there might be better ways to handle those. What are your suggestions?

Adding definition for those functions and importing them at the top of each file that use them?

Or were you thinking of something else such as using globals and/or externals in webpack?

zachmullen commented 7 years ago

Adding definition for those functions and importing them at the top of each file that use them?

I like that idea, it keeps the modularity consistent. Then I'd suggest having some function or functions to control the logging levels. Calling it at runtime would set some variable in the logging module that would render some or all of the methods into no-ops.

jourdain commented 7 years ago

Instead of using variable, this could be managed using alias in the webpack config, which could also rely on environment variable to decide which one should be imported for what.

zachmullen commented 7 years ago

That would be cleaner for sure. It would still require downstreams to replicate the alias configuration, but maybe we could provide some way to make that easier via a build-time helper module that ships with vtk.js.

waxlamp commented 7 years ago

If it helps, you can check out what I did for Candela. It seems to work pretty well as a "build-time helper module": https://github.com/Kitware/candela/blob/webpack-helper/webpack.js

roni

On Wed, Feb 1, 2017 at 10:13 AM Zach Mullen notifications@github.com wrote:

That would be cleaner for sure. It would still require downstreams to replicate the alias configuration, but maybe we could provide some way to make that easier via a build-time helper module that ships with vtk.js.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Kitware/vtk-js/issues/108#issuecomment-276682759, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxNJFTJeKaMME-JdNZwuuHPvCNurtYOks5rYKD7gaJpZM4Lz8C3 .

jourdain commented 7 years ago

Thanks @ronichoudhury and @zachmullen, I'll try to tackle it and come up with a solution.