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

Add config file support to @wordpress/env #18121

Merged
merged 9 commits into from
Oct 30, 2019
Merged

Add config file support to @wordpress/env #18121

merged 9 commits into from
Oct 30, 2019

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Oct 26, 2019

Note: This PR has been superseded by #20002, so follow the information there instead.

Description

With this PR, a user can specify a wp-env.json file with local dependencies for the tool to load. The goal is to make life easier for plugin devs who need to be working on multiple plugins and/or themes at the same time.

  1. During the wp-env start command, look in the local directory for a wp-env.json file.
  2. Parse the JSON out of this file into a plugins and themes array.
  3. Use detect-context to create context objects for each dependency path. This is the same format used for the local context, which makes things easy. I also added the absolute path to the dependency and basname of the path to the context object so that they can be reused easily.
  4. Map each dependency to the Docker config file as a volume.

Here is a JSON file I used to test this:

wp-calypso/apps/full-site-editing/full-site-editing-plugin/wp-env.json

{
    "themes": [
        "../../../../themes/varia",
        "../../../../themes/maywood"
    ],
    "plugins": [
        "../../../../gutenberg"
    ]
}

That's pretty nasty with the relative paths, but I wanted all developers of the plugin to use it if the repos are in the same top-level directory. For me this is source/gutenberg, source/wp-calypso, etc. Home paths (i.e. ~/source) don't work since the tilde is not supported in fs, and absolute paths wouldn't be flexible for other devs. I'm open to a better approach here if folks have ideas. :)

How has this been tested?

I verified locally that the specified plugins and themes in the wp-env.json file were loaded into the local WordPress install created by wp-env start.

Types of changes

New Feature.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

cc @epiqueras

See here: #17724 (comment)

We will use this info to build the docker config object
It resolves the dependencies from the .wpenv file and returns them in the
same "context" format as detect-context.
Autopopulates the docker config with volume mappings for the dependency contexts.
Copy link
Contributor

@epiqueras epiqueras left a comment

Choose a reason for hiding this comment

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

Great work here, almost ready to merge!

packages/env/lib/create-docker-compose-config.js Outdated Show resolved Hide resolved
packages/env/lib/detect-context.js Outdated Show resolved Hide resolved
packages/env/lib/detect-context.js Outdated Show resolved Hide resolved
packages/env/lib/detect-context.js Outdated Show resolved Hide resolved
packages/env/lib/env.js Show resolved Hide resolved
packages/env/lib/resolve-dependencies.js Outdated Show resolved Hide resolved
- Rename pathName to pathBasename
- Use .wp-env instead of .wpenv
- Inline useless function
- Clarify default param in jsdoc for detectContext
@noahtallen
Copy link
Member Author

Thanks @epiqueras! I resolved your feedback and added dependency activation to the clean command. Ready for another look now :)

@youknowriad
Copy link
Contributor

I might be a bit late here but I wonder about supporting the generic .env file (it's something a lot of tools and projects are adopting) instead of a custom wpenv config?

I believe we already make use of environment variables in some of these configs, so it seems like a natural fit?

@epiqueras
Copy link
Contributor

@noahtallen Thanks for the iteration, one last thing to fix.

@youknowriad Using environment variables for arrays is really hard to wrangle and is a ripe incompatibility and conflict area in different OSs. Furthermore, I wouldn't be surprised if WP requires us to add a lot more options down the road. What I do think we should do is add the .json file extension to make sure all editors pick up on the format out of the box. cc @noahtallen

@noahtallen
Copy link
Member Author

@epiqueras Something like wp-env.json instead of .wp-env?

@noahtallen
Copy link
Member Author

I removed dependency activation for the test environment clean :)

@epiqueras
Copy link
Contributor

@epiqueras Something like wp-env.json instead of .wp-env?

Exactly. What do you think? It seems like most tools are gravitating towards this now to be explicit. Just looking at other config files in this repo supports this.

@noahtallen
Copy link
Member Author

I have no strong feelings on the issue. I don't see a strong reason to use either, though the automatic json detection is nice.

Here's the wp-calypso repo as an example of a large JS app:
Screen Shot 2019-10-29 at 11 34 48 AM

The .* format for config files seems pretty standard, but I don't think it really affects developers. They'll have to follow documentation either way :D

@youknowriad
Copy link
Contributor

docker-compose and docker are already relying on .env files and it's one of the underlying tools for @wordpress/env. more and more tools are using this pattern.

I see two main pros:

  • no new format to learn, just variables names
  • the growing list of config files at the root level of different projects makes me nervous :)

I agree that arrays are difficult but there are workarounds (comma separation...).
I have a preference from .env files but I respect your decisions here. It is also something we can reconsider in the future.

@epiqueras
Copy link
Contributor

Docker uses YAML files.

.env files are for global environment variables not Docker configuration options.

We would also have to prefix every variable to avoid conflicts. It's just not practical.

@epiqueras
Copy link
Contributor

@noahtallen Let's go with JSON for the detection reasons.

@noahtallen
Copy link
Member Author

done

packages/env/lib/resolve-dependencies.js Outdated Show resolved Hide resolved
Co-Authored-By: Enrique Piqueras <[email protected]>
@noahtallen
Copy link
Member Author

@epiqueras Feel free to merge :) (I don't have access)

@epiqueras epiqueras merged commit 43db91b into WordPress:master Oct 30, 2019
@noahtallen noahtallen deleted the add/wp-env-config-file-support branch October 30, 2019 23:01
@talldan talldan added the [Tool] Env /packages/env label Nov 6, 2019
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
@epiqueras
Copy link
Contributor

We need to update the README for this.

@noahtallen
Copy link
Member Author

@epiqueras true! Here it is: #18643

@epiqueras
Copy link
Contributor

Thank you!

@jorgefilipecosta jorgefilipecosta added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 4, 2020
@mcsf
Copy link
Contributor

mcsf commented Feb 7, 2020

This PR needs a post-merge update of its description to reflect the correct filename instead of wpenv.

@noahtallen
Copy link
Member Author

I've updated the description to use wp-env.json instead of .wpenv (the filename at the time of merge). I also added a note that this PR has now been superseded by #20002, which changes it to .wp-env.json

@jorgefilipecosta jorgefilipecosta added Needs Dev Note Requires a developer note for a major WordPress release cycle and removed Needs Dev Note Requires a developer note for a major WordPress release cycle labels Feb 11, 2020
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
23 tasks
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Env /packages/env [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants