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

Update optimizer webpack config to support ReactFlow dependency #4882

Merged
merged 7 commits into from
Sep 22, 2023

Conversation

ohltyler
Copy link
Member

Description

This PR adds MIT-licensed ReactFlow 11.8.3 as a dependency. It also updates the webpack configuration to use the proper babel loader, very similarly to what was needed with the vega v5 dependency.

We want to add this dependency into core OSD for a few reasons:

  1. We will be using this library in a new OSD plugin as specified in the RFC. This will be used for stitching together OpenSearch components and developing applications.
  2. There is a likelihood that some/all of this logic will be moved into OSD eventually, so we can skip some migration work by adding here directly
  3. Prevents multiple versions of the same library maintained by other plugins by persisting into core OSD to start
  4. The complexities around transpiling correctly and maintaining special loaders and webpack/babel configurations in the plugin isn't sustainable and adds a lot of special complexity.

Testing the changes

I've tested out using ReactFlow in an external plugin to ensure it works as expected, and the dependency is pulled correctly.

Check List

  • Commits are signed per the DCO using --signoff

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #4882 (3ec840d) into main (a10f1c7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4882   +/-   ##
=======================================
  Coverage   66.50%   66.50%           
=======================================
  Files        3403     3403           
  Lines       65026    65026           
  Branches    10401    10401           
=======================================
  Hits        43245    43245           
  Misses      19213    19213           
  Partials     2568     2568           
Flag Coverage Δ
Linux_1 34.82% <ø> (ø)
Linux_2 55.31% <ø> (ø)
Linux_3 44.61% <ø> (ø)
Linux_4 34.89% <ø> (ø)
Windows_1 34.83% <ø> (ø)
Windows_2 55.27% <ø> (ø)
Windows_3 44.61% <ø> (ø)
Windows_4 34.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ashwin-pc
Copy link
Member

ashwin-pc commented Sep 8, 2023

This is a personal opinion, but i'd avoid adding dependencies to core unless core requires it too. Yes this requires updates to multiple plugins separately, but i'd rather not introduce anti patterns instead of solving the underlying problem, which is the dependency hell that we have. And doing it this way makes us less inclined to solve that.

That being said, since the core problem here is that webpack need to transpile the source code because it does not have native es5 builds, i'm okay making an exception here. But before doing so i'd really like to know if just adding the same webpack changes to plugins would work. A quick exploration into this might be worth it since i dont expect this to be the last time we do something like this.

This PR adds a lot of transitive dependencies to the repo because of this npm module. That means that we also have a lot of potential security vulnerabilities that we have to watch out for and maintain. Id love to minimize that if possible since dependencies added here affect every plugin.

@AMoo-Miki @joshuarrrr what are your thoughts?

package.json Outdated Show resolved Hide resolved
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@joshuarrrr
Copy link
Member

I'll start with the provided reasons (thanks @ohltyler for the helpful summary):

We want to add this dependency into core OSD for a few reasons:

  1. We will be using this library in a new OSD plugin as specified in the RFC. This will be used for stitching together OpenSearch components and developing applications.
  2. There is a likelihood that some/all of this logic will be moved into OSD eventually, so we can skip some migration work by adding here directly
  3. Prevents multiple versions of the same library maintained by other plugins by persisting into core OSD to start
  4. The complexities around transpiling correctly and maintaining special loaders and webpack/babel configurations in the plugin isn't sustainable and adds a lot of special complexity.

I don't find 1-3 to be compelling for adding many new core dependencies. 3 hints at the idea that, eventually, multiple different plugins will need to build on top of ReactFlow, but the RFC as of today seems to call for just a single plugin to start. I also think it philosophically goes against the spirit of a minimal OpenSearch Dashboards distribution that's further extended via plugins. So I'm in agreement with @ashwin-pc's concerns above.

However, reason 4 definitely resonates with me - I can see why this might be a pain at the plugin level, where there's much less visibility and control over OpenSearch Dashboards' complex loading and bundling infrastructure. I agree with Ashwin that I'd like to see if webpack configuration at the plugin level is sufficient. If not, I'd be OK with this in main, and not backported, with the focus on the Webpack 5 migration instead: #2875, #1118

@joshuarrrr
Copy link
Member

note also that React Flow will soon have a naming change: https://wbkd.notion.site/Upcoming-Changes-at-React-Flow-1a443641891a4069927c0a115e915251

@ohltyler
Copy link
Member Author

@joshuarrrr @ashwin-pc thanks both for the feedback, that all makes sense to me. Regarding the webpack 5 migration, is that going to be something prioritized? And if not, if we want to release the plugin in the 2.x series, how should we go about that if this PR is not backported?

@ashwin-pc
Copy link
Member

@ohltyler Here are a few options:

  1. Lets give the webpack changes in the plugin a try. based on the changes in the PR, i think doing the same in the plugin's webpack file should work just as well. But we can validate that easily enough.
  2. Have you considered alternative dependencies? like Reaflow.dev?

@joshuarrrr why cant this be backported?

@ohltyler
Copy link
Member Author

ohltyler commented Sep 14, 2023

@ohltyler Here are a few options:

  1. Lets give the webpack changes in the plugin a try. based on the changes in the PR, i think doing the same in the plugin's webpack file should work just as well. But we can validate that easily enough.
  2. Have you considered alternative dependencies? like Reaflow.dev?

@joshuarrrr why cant this be backported?

Right now, the plugin isn't taking on any build/compile-related package dependencies, like webpack or babel. Any build or bootstrap commands (yarn build, yarn osd bootstrap etc.) are linked to the ones offered in core OSD (ref). Are you suggesting now taking on individual build packages at the plugin level to try to resolve this? Would this mean removing the links to osd etc.?

@AMoo-Miki
Copy link
Collaborator

AMoo-Miki commented Sep 21, 2023

In addition to the great points others brought up, for me it all boils down to one thing: OSD is not using it so it is not a dep of OSD and needn't be added as such; OSD cannot afford to maintain a package that it doesn't depend on. I understand where you are coming from and I see how that could simplify things for "some" plugins but with the same logic, it would make it challenging (and even blocking) for other plugins. Keep in mind that OSD will not be able to bump that package without incurring a breaking change that requires a major version bump.

Is there something that your plugin is unable to achieve on its own without this being a dep of OSD?

@ohltyler
Copy link
Member Author

In addition to the great points others brought up, for me it all boils down to one thing: OSD is not using it so it is not a dep of OSD and needn't be added as such; OSD cannot afford to maintain a package that it doesn't depend on. I understand where you are coming from and I see how that could simplify things for "some" plugins but with the same logic, it would make it challenging (and even blocking) for other plugins. Keep in mind that OSD will not be able to bump that package without incurring a breaking change that requires a major version bump.

Is there something that your plugin is unable to achieve on its own without this being a dep of OSD?

Our plugin does not maintain any build/compile-related packages like I've mentioned in the previous comment. If this is a supported pattern, or if there is any existing plugin that is doing such, that would be helpful. I'm very worried in the complexity we would need to build some things some way from core OSD, and build other things other ways using a plugin-customized webpack & babel configuration, as well as an entirely new set of build scripts in package.json to build both simultaneously when bootstrapping the plugin inside of core OSD.

@ohltyler
Copy link
Member Author

To me it seems the much bigger issue is that OSD is not on webpack 5 so it will become more and more common where packages will start to cause more issues when they don't support es5 builds. If that was resolved we could much more easily add the package in our plugin since the OSD build packages would support it out of the box.

@AMoo-Miki
Copy link
Collaborator

AMoo-Miki commented Sep 21, 2023

Our plugin does not maintain any build/compile-related packages like I've mentioned in the previous comment. If this is a supported pattern, or if there is any existing plugin that is doing such, that would be helpful.

If a plugin, made by OpenSearch or someone else, needs a specific build system or has specific dependencies, they are responsible for creating and maintaining it. For example, ISM until recently provided their own babel.config.js. Search Relevance have their own webpack.config.

Additionally, I can support the change to packages/osd-optimizer/src/worker/webpack.config.ts in this PR if that unblocks you, but I cannot support adding a dependency that is not used by OSD.

I'm very worried in the complexity we would need to build some things some way from core OSD, and build other things other ways using a plugin-customized webpack & babel configuration, as well as an entirely new set of build scripts in package.json to build both simultaneously when bootstrapping the plugin inside of core OSD.

Plugins are free to build themselves however they wish to. OSD has provided some tools to ease the burden but if a plugin finds those tools inadequate for their unique use case, they are more than welcome to build and package on their own or include config files stored in their codebase that get picked up by OSD's tools.

To me it seems the much bigger issue is that OSD is not on webpack 5 so it will become more and more common where packages will start to cause more issues when they don't support es5 builds. If that was resolved we could much more easily add the package in our plugin since the OSD build packages would support it out of the box.

That is indeed a problem and we are focused on solving it for the next major version (since it would be a breaking change).

@ohltyler
Copy link
Member Author

@AMoo-Miki thanks a lot for those details. I've POC'd where if the webpack.config changes are done here, but ReactFlow is added as a dep in the plugin, it works as expected. I think this will be the simplest approach as it avoids having to take on a plugin-standalone build environment, and once webpack 5 is available, even that extra config in here can be removed.

@ohltyler ohltyler added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Sep 21, 2023
@ohltyler ohltyler changed the title Add ReactFlow as a dependency Update optimizer webpack config to support ReactFlow dependency Sep 21, 2023
@AMoo-Miki
Copy link
Collaborator

I've POC'd where if the webpack.config changes are done here, but ReactFlow is added as a dep in the plugin, it works as expected.

Just curious as I have not tried this myself: did you experiment with adding the webpack.config to the plugin? I would love to learn from your experience with that.

@joshuarrrr joshuarrrr merged commit eec3ed7 into opensearch-project:main Sep 22, 2023
103 of 104 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 22, 2023
* Add reactflow with compiler fix

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>

* Update CHANGELOG

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>

* Update comment

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>

* Revert adding reactflow dep

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>

---------

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Co-authored-by: Miki <miki@amazon.com>
(cherry picked from commit eec3ed7)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@ohltyler ohltyler deleted the reactflow-fix branch September 22, 2023 21:44
@ohltyler
Copy link
Member Author

I've POC'd where if the webpack.config changes are done here, but ReactFlow is added as a dep in the plugin, it works as expected.

Just curious as I have not tried this myself: did you experiment with adding the webpack.config to the plugin? I would love to learn from your experience with that.

I tried a bit with some custom build configurations but I couldn't get things to work. I'm not familiar enough with the combination of nested projects with webpack & babel, or how to integrate with the OSD build system enough.

joshuarrrr pushed a commit that referenced this pull request Sep 23, 2023
… (#5094)

* Add reactflow with compiler fix



* Update CHANGELOG



* Update comment



* Revert adding reactflow dep



---------



(cherry picked from commit eec3ed7)

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Miki <miki@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
all-star-contributor backport 2.x dependencies Pull requests that update a dependency file Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants