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

fix(atomic): make sure Stencil start on the good port #697

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

louis-bompart
Copy link
Collaborator

Proposed changes

What's going on

Netlify even tho it does some framework detection, does not negotiate the 'targetPort' with the frameworks.
To understand why, we have to understand a bit how Netlify works, which is quite simple: As our CLI.
By that, I mean that the Netlify-CLI will do the same thing that we do: start other CLI (like stencil-cli in this instance).

However, what it doesn't do is the 'port mapping' between the option given in input and the underlying CLIs.

Proposed fix

To alleviate that issue, I circumvent the Netlify-CLI by relying on the fact that it's good practice to 'carry the parent process.env around when spawning child processes.
This allows me to spawn netlify-cli with the STENCIL_PORT in its env, and to get it back when the stencil config is evaluated.

We still pass targetPort so that when netlify-cli wait for the port of the framework to be open, it waits on the good one.

Testing

Current FTs are enough, tho for some obscure reason, it was working on Linux. (like, the expected behaviour from our perspective was OK, the fact that it worked in Netlify is pretty nebulous tho).

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2022

Thanks for your contribution @louis-bompart !
When your pull-request is ready to be merged, check the box below to merge it

  • Merge! :shipit:

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2022

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Copy link
Contributor

@ThibodeauJF ThibodeauJF left a comment

Choose a reason for hiding this comment

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

Very elegant, thank you 💯

@louis-bompart louis-bompart merged commit 662746c into master Mar 1, 2022
@louis-bompart louis-bompart deleted the CDX-858 branch March 1, 2022 15:06
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.

3 participants