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.

Sign up for GitHub

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

Platform Docs: Update intro.md #60087

Closed
wants to merge 4 commits into from
Closed

Platform Docs: Update intro.md #60087

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe80dfe
Update intro.md with instructions for configuring build tooling, sett…
michalczaplinski Mar 21, 2024
8bd0748
Update intro.md with Gutenberg Block Editor setup
michalczaplinski Mar 21, 2024
f6dd539
Merge branch 'trunk' into update/platform-docs-intro
michalczaplinski May 13, 2024
015a33f
Use the react preset and split into main.jsx and Editor.jsx
michalczaplinski May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions platform-docs/docs/intro.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,39 @@ To build a block editor, you need to install the following dependencies:

We're going to be using JSX to write our UI and components. So one of the first steps we need to do is to configure our build tooling, By default vite supports JSX and and outputs the result as a React pragma. The Block editor uses React so there's no need to configure anything here but if you're using a different bundler/build tool, make sure the JSX transpilation is setup properly.

## Side note: `process.env.IS_GUTENBERG_PLUGIN`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should fix this in Gutenberg directly. We should make sure that it works by default without setting this rather than having npm consumers learn about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that makes sense! I see that this is actually a GB issue because it should have been replaced at build time and we even have a lint for it.

Do you have any idea why that would not be the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that the version of the packages published to npm does not have the value of process.env.IS_GUTENBERG_PLUGIN replaced. E.g.: You can check here: https://unpkg.com/browse/@wordpress/[email protected]/build/resolvers.js

Copy link
Contributor

Choose a reason for hiding this comment

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

@michalczaplinski yes, that's fine, because Gutenberg also consumes the built npm packages like any other app. The problem is that it shouldn't cause an error by default but should assume "false" if you don't define the variable.


The Gutenberg block editor reads from the `process.env.IS_GUTENBERG_PLUGIN` variable. It
is used to distinguish the code that is part of the Gutenberg block editor
[plugin](https://wordpress.org/plugins/gutenberg/) and the code that is part of
WordPress core.

For our purposes, as we're building a standalone block editor that is completely
separate from WordPress, we want to set this variable to `false`.

When using Vite, we can do it by creating a `vite.config.js` file:

```js
// vite.config.js
import { defineConfig } from "vite";

export default defineConfig({
define: {
"process.env.IS_GUTENBERG_PLUGIN": JSON.stringify(false),
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't think we should add this to the docs, it doesn't make sense for third-party developers and shouldn't be required.

Copy link
Member

@oandregal oandregal Apr 26, 2024 •
edited by swissspidy
Loading

Choose a reason for hiding this comment

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

I looked a bit into this and found that a similar request for the @wordpress/warning package was addressed by creating a SCRIPT_DEBUG global. There was a conversation about an alternative approach that was similar to what I was thinking to fix this specific use case for the block library.: process?.env?.IS_GUTENBERG_PLUGIN ?? false.

My understanding is that we cannot use import.meta unless we ship our scripts as modules. Both vite and webpack favor this approach these days, but people find it problematic with many libraries and end up defining the process.env themselves — like we do in Gutenberg and people recommend for vite.

This is the options I see:

  1. Document in the framework docs (this PR).
  2. Make it not to break just for the @wordpress/block-library package: process?.env?.IS_GUTENBERG_PLUGIN ?? false. I don't find this approach sustainable in the whole codebase, though.
  3. Update the variable to a IS_GUTENBERG_PLUGIN global.

I'd like to summon @gziolo @jsnajdr @swissspidy @talldan @t-hamano that have participated in similar conversations, in case they have thoughts/advice.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that we cannot use import.meta unless we ship our scripts as modules.

FWIW, I think we already publish our scripts as modules. So is the idea to use process.env for the CJS build and import.meta for the modules build? That might be the best approach no?

Copy link
Member

Choose a reason for hiding this comment

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

IDK why Gutenberg uses process.env.IS_GUTENBERG_PLUGIN, but just using a IS_GUTENBERG_PLUGIN global (like the SCRIPT_DEBUG one that was added recently) makes more sense. Web scripts shouldn't artificially introduce a process.env that doesn't exist. Also weird that the global isn't replaced in build scripts 🤔

like we do in Gutenberg and vitejs/vite#1973.

Tip: when linking to files on GitHub, copy its permalink via the three-dots menu or press Y on the keyboard to get the permalink. This way we can see the file in the version at the time of writing, even when the file is later modified or moved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Web scripts shouldn't artificially introduce a process.env that doesn't exist

For bundled scripts yes, but packages can ship and support environment variables. process.env variables have always been the standard in the JS community for env variables but lately import.meta is becoming the standard when it comes to ES modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'd love if we can experiment with this #60087 (comment) and use consistently for all of our env variables. SCRIPT_DEBUG is a runtime variable, so it's different.

That said, I don't want to block this PR more. I'm fine with mentioning the variable in our docs temporarily while we figure this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned it in slack, but will mention it here too. There's also usage of process.env.NODE_ENV in the codebase (in packages like blocks, components, data).

I don't know if build tools handle that property in a special way or if it also causes the same issues as IS_GUTENBERG_PLUGIN. It might be the latter, but it's just that IS_GUTENBERG_PLUGIN causes an error first.

Just mentioning it as I'd hate to see effort go into fixing IS_GUTENBERG_PLUGIN but then later realize there's still a problem with NODE_ENV.

},
});
```

If you are using Webpack, you can use the
[DefinePlugin](https://webpack.js.org/plugins/define-plugin/) to accomplish the
same thing.

## Bootstrap your block editor

It's time to render our first block editor.
It's time to render our first block editor. Update your `index.jsx` file with the following code:
Copy link
Contributor

@stokesman stokesman May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When using "Vanilla" starter index.html imports main.js and there is no index.jsx. Additionally, since Vite doesn’t automatically apply JSX transforms to js files we’d probably want to do something like:

Suggested change
It's time to render our first block editor. Update your `index.jsx` file with the following code:
It's time to render our first block editor. Rename your `main.js` file to `main.jsx` and update it with the following code:

It’s one more small reason to prefer the "React" starter as relevant files will already be .jsx. Then we can say to put the code in src/App.jsx and also remove the bits about createRoot and render because those will already exist in src/main.jsx.

I'd be happy to put the switch to "React" starter in a follow-up PR though because, well, like a goof I already created one that does that (and has many of the changes in this one) because I didn’t check first to see if it was being worked on.


- Update your `index.jsx` file with the following code:
```jsx
import { createElement, useState } from "react";
import React, { useState } from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine but I kind of thought that vite uses the automatic JSX config by default. it's not the case?

Copy link
Contributor Author

@michalczaplinski michalczaplinski Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are more details in https://v2.vitejs.dev/guide/features.html#jsx but TL;DR is that it's not the case out of the box. We can also do one of those things

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it simple and just import React like you did for now.

Copy link
Member

Choose a reason for hiding this comment

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

If you create the Vite app with the "React" / "JavaScript" config instead of "Vanilla" / "JavaScript" config, this works automatically. The vite config becomes:

import { defineConfig } from 'vite'
import react from '@vitejs/plugin-react'

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react()],
});

Copy link
Member

Choose a reason for hiding this comment

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

Actually, starting with the "Vanilla" vite config cerates a few issues (jsx, react), why not change the docs to suggeste starting with the "React" vite config? Those things will be solved.

Copy link
Contributor

Choose a reason for hiding this comment

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

If starting with "Vanilla" then react and react-dom aren’t added as dependencies so it seems like that would warrant mentioning them in "Installing dependencies" section.

I'd favor instructing to use the "React" variant. It seems like it simplifies the instructions and provides a better starting place as it adds some helpful packages like eslint with react-related plugins.

import { createRoot } from 'react-dom/client';
import {
BlockEditorProvider,
Expand Down
Loading