refactor: esbuild refactor for readability

[torresxb1]

Issue #, if available:

Description of changes:
I originally intended to add build-in-source support for the esbuild workflow but I found it hard with the way it was currently set up. I'm hoping my changes make it more readable and easier to work with in general, but let me know if you disagree with my changes or you think there could be further improvements.

Main changes done:

• Changed the bundle action:
  • parameter names were kind of coupled to scratch and artifacts directory, so I changed them to accept a working directory and an output directory
  • it now performs the esbuild version checking as part of this action (when needed)
• workflow conditionals/IFs cleanup: my main issue with the workflow was the accelerate workflow stuff was a bit complicated, with a bunch of conditions and nested ifs that went a few levels deep. In my proposed changes for the workflow, IFs would only go a single level deep.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[torresxb1]

This used to be a separate action. Since we need to check the version every time that we want to mark things as --external then I think we should do the check when/before marking things as external (here), instead of having a separate action that would always have to be appended before this one. This cleans things up a bit, but it's also safer as we don't have to remember that we need a check action before this bundle action.

[torresxb1]

renamed to make more general

[torresxb1]

This file changed quite a bit. Might be easier to open the new and the old file in different windows (without the file diffs), and comparing.

[torresxb1]

I think using named arguments can be more readable in cases where it's not immediately obvious what the parameter(s) would be for. E.g. here we would immediately know which folder we copy from and which one we copy to, without having to check the CopySourceAction definition.

[torresxb1]

combine_dependencies determines whether we should bundle dependencies or not

[torresxb1]

it would be better if this argument was named install_dir, as it actually controls where dependencies are installed. Will rename in another PR, when I introduce building-in-source to esbuild

[torresxb1]

a couple tests will have one action less because I moved the action to check esbuild version into the bundling action itself.

[torresxb1]

the order did change, to simplify the code logic. There should be no impact in the end-result, however. We just need to clean up the dependencies dir before we move the updated dependencies in there, but the bundling itself happens on another folder.

[mndeveci]

Thanks for the effort! Left some comments but will definitely do another round.

[mildaniel]

Thanks Rup, this is a great QoL change. Left a couple small comments but generally agree with the direction you took combining the bundle action and version check action since they are always bundled together.

[mildaniel]

Seems that the bundle action is already included in line 103, so this would run the bundler twice.

[torresxb1]

This is one part that can lead to confusion and wasn't able to figure out how to make it better. It would never bundle twice because this bundle on this line happens when self.download_dependencies is False, while the one in line 103 happens when self.download_dependencies is True

[torresxb1]

in other words, if we're reusing dependencies, then we didn't download them and we should first symlink and then bundle. If we downloaded dependencies, then we should bundle after downloading, (and we won't symlink or bundle again)

[mildaniel]

I'm seeing it if it's true for both cases:

if self.download_dependencies:
            self.actions += [
                NodejsNpmWorkflow.get_install_action(
                    source_dir=source_dir,
                    artifacts_dir=self.scratch_dir,
                    subprocess_npm=self.subprocess_npm,
                    osutils=self.osutils,
                    build_options=self.options,
                ),
                bundle_action,
            ]

        if not self.dependencies_dir:
            # we need a dependencies dir for the logic below
            return

        if self.download_dependencies:
            # if we downloaded dependencies, we have to update dependencies_dir
            self.actions += [
                CleanUpAction(self.dependencies_dir),
                MoveDependenciesAction(self.source_dir, self.scratch_dir, self.dependencies_dir),
            ]
        else:
            # if we're reusing dependencies, then we need to symlink them before bundling
            self.actions += [
                LinkSourceAction(self.dependencies_dir, self.scratch_dir),
                bundle_action,
            ]

[torresxb1]

I think your indentation might be off? 😅 . The first if in the code block you're posting should be in the same level as the other two if's below

[torresxb1]

or otherwise not sure what you mean

[torresxb1]

I just made an additional change related to this, hopefully it's clearer now.

[torresxb1]

@mndeveci added tests to increase coverage and changed to numpy docstrings

[mndeveci]

Thanks for the effort that you put here @torresxb1! Approving the changes since we confirm that expected actions are been setup with possible combinations through tests.