Skip to content

Conversation

@erunion
Copy link
Member

@erunion erunion commented Oct 25, 2023

🧰 Changes

The prepare script that was added to codegen'd package.json files in #761 is being executed when we install that SDK resulting us in running the compilation step twice. Because I'd like to keep the dependency installation and SDK dist compilation processes separate we should run npm install with the --ignore-scripts option.

fixes RM-8243

@erunion erunion added bug Something isn't working area:api Issues related to the `api` CLI, which builds the SDKs labels Oct 25, 2023
@erunion erunion requested a review from kanadgupta October 25, 2023 23:27
@erunion erunion marked this pull request as ready for review October 25, 2023 23:27
Copy link
Contributor

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i added a little prompt after this line to confirm that the dist/ directory wasn't being built before the compilation step, but it looks like it's being built:

CleanShot 2023-10-26 at 09 34 26@2x

i did a little digging and was able to fix this by making this change on this line:

-    return execa('npm', [...npmInstall, installDir].filter(Boolean))
+    return execa('npm', [...npmInstall].filter(Boolean), { cwd: installDir })

@erunion
Copy link
Member Author

erunion commented Oct 26, 2023

Is the api in that case added to the root package.json file? I think thats just going to run npm i install the api and not add it as a dependency

@kanadgupta
Copy link
Contributor

I think thats just going to run npm i install the api and not add it as a dependency

ah shit you're right 😬 my hunch is that the --ignore-scripts flag only applies to scripts in the package.json where you're running npm i from and it runs all the lifecycle scripts in the subdirectory's package.json

@kanadgupta
Copy link
Contributor

another idea:

  1. build the initial package.json without a prepare script
  2. add the prepare script during the compilation step using npm pkg set?

Comment on lines +207 to +208
.then(res => handleExecSuccess(res, opts))
.catch(err => handleExecFailure(err, opts));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nice to write a small wrapper for execa() that handles these success and failure states instead of having to copy-paste this code everywhere but i don't really have the energy at the moment to try to figure out how to clone all of execa's parameter typing into a wrapper

@erunion erunion requested a review from kanadgupta October 26, 2023 17:21
Copy link
Contributor

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was able to confirm it works as expected with the fix below!

const REF_PLACEHOLDER = '::convert::';
const REF_PLACEHOLDER_REGEX = /"::convert::([a-zA-Z_$\\d]*)"/g;

function handleExecSuccess(res: ExecaReturnValue<string>, opts: InstallerOptions = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great refactor here 🫶🏽

Co-authored-by: Kanad Gupta <8854718+kanadgupta@users.noreply.github.com>
@erunion erunion requested a review from kanadgupta October 26, 2023 17:33
@erunion erunion merged commit f93f152 into main Oct 26, 2023
@erunion erunion deleted the fix/dont-run-scripts branch October 26, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:api Issues related to the `api` CLI, which builds the SDKs bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants