docs: v12 migration guide [LIBS-689] #884
Open
+980
−776
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kabaros
reviewed
Oct 22, 2024
kabaros
reviewed
Oct 22, 2024
|
|
||
| - Environment variables on `process.env` will be dropped in future versions, in favor of variables on `import.meta.env` | ||
| - The default `direction` config value is `ltr`; in the future, it will be `auto` | ||
| - `--allowJsxInJs` will be removed |
There was a problem hiding this comment.
wil this be removed in the underlying Vite CLI?
There was a problem hiding this comment.
Not sure which bullet point you mean haha, but here's an answer for each:
- Env vars are under our control, but I'm actually rethinking this point, i.e. maybe we should keep variables on
process.env-- Earlier, I thoughtimport.meta.envwas a pretty standard thing with ES6 and that we should migrate to that for env vars, but I think that's actually just a Vite pattern, so it might be "more standard" to keep env vars onprocess.env. If we do that, we can at least deprecate theREACT_APP_prefixes for the variable keys - (Probably obvious, but direction is also under our control)
- For
--allowJsxInJs, that's also under our control -- it's a flag that our own CLI reads, and adds a bunch of kinda complex Vite config inmakeViteConfig.mjs. That additional config may or may not be supported in future versions of Vite, but I think we should remove our--allowJsxInJsflag and config in the nextcli-app-scriptsversion to make things simpler, standardized, and more performant
kabaros
reviewed
Oct 24, 2024
kabaros
reviewed
Oct 24, 2024
Topener
reviewed
Oct 29, 2024
| @@ -1,6 +1,8 @@ | |||
| - [**Getting Started**](getting-started.md) | |||
| - [**Installation**](installation.md) | |||
| - [**Bootstrapping**](bootstrapping.md) | |||
| - [**Migration**](migration) | |||
There was a problem hiding this comment.
This sidebar isn't used, you need to add it to the sidebar in the developer portal!
So after merging this PR you'd need to merge the developer portal PR as well to get it to show up.
There was a problem hiding this comment.
Ah okay, thanks for the reminder!
Topener
requested changes
Oct 30, 2024
| @@ -0,0 +1,329 @@ | |||
| # Platform v12 migration guide: Vite & React 18 | |||
|
|
|||
| Good news! Vite and React 18 in the app platform are ready to use! | |||
There was a problem hiding this comment.
This line, while good and happy, is very much "timed" as if for a blogpost or the announcement. If someone reads this guide in half a year or later it will be odd. Not sure if it can be omitted fully or reworded.
Paragraph below could use a little rewording as well to match this sentiment.
docs/migration/v12.md
Outdated
|
|
||
| Here are some edge cases that were encountered when upgrading some apps with the new platform version: | ||
|
|
||
| - There is a bug in Vite: importing from a directory, e.g. `‘./app/‘` where we would assume to get `./app/index.[jt]s`, can incorrectly resolve to a file with a similar name to the directory, e.g. `./App.tsx`. I see this as a bug with Vite, but as a workaround, file and dir names can be changed so they don’t conflict. Noticed in the Maintenance app beta. |
There was a problem hiding this comment.
Can the Maintenance app example be linked/explained?
docs/migration/v12.md
Outdated
|
|
||
| - There is a bug in Vite: importing from a directory, e.g. `‘./app/‘` where we would assume to get `./app/index.[jt]s`, can incorrectly resolve to a file with a similar name to the directory, e.g. `./App.tsx`. I see this as a bug with Vite, but as a workaround, file and dir names can be changed so they don’t conflict. Noticed in the Maintenance app beta. | ||
| - If you’re using `identity-obj-proxy` in your test config, make sure you add it to the project’s dependencies. Previously, some apps got away with using the proxy without an explicitid dependency because the package is a dependency of `react-scripts` | ||
| - You may see some build warnings coming from the `mathjs` package. Upgrading to a recent version of `mathjs` will handle these warnings. Noticed in analytics app (the analytics library has a dependency on `mathjs`) |
There was a problem hiding this comment.
Does the mention of the Analytics app matter? It might confuse more than solve as the solution is a simple upgrade of a package. Maybe add the command to handle this upgrade instead?
docs/migration/v12.md
Outdated
| 9. An `App.jsx` entrypoint is no longer quietly added if a plugin entrypoint is defined or for a defined but empty entrypoints object | ||
| 10. Plugins and apps are started together in the same process on the same port | ||
| 11. Custom `index.html` files are no longer supported | ||
| 12. `import * as MyClass from ‘my-module’` is now more restrictive, and may break in some cases. So far, changing the import to `import MyClass from ‘my-module’` has fixed it. DOMPurify is one example case, noticed in the Login App |
There was a problem hiding this comment.
Can Login App example be linked/explained?
Co-authored-by: Rene Pot <[email protected]>
Co-authored-by: Rene Pot <[email protected]>
Co-authored-by: Rene Pot <[email protected]>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LIBS-689