Conversation
benpryke
left a comment
benpryke
left a comment
There was a problem hiding this comment.
To parse my comments, it may help if you read t he documents in the order I reviewed them in, which was
- microfrontends.md
- add.md
- esm views.md (should this have a hyphen?!)
docs/concepts/microfrontends.md
Outdated
|
|
||
| ## ESM micro frontends in Modular | ||
|
|
||
| Micro frontends are a pattern where discrete UIs (frontends) are composed from |
There was a problem hiding this comment.
Grammar:
- pattern in which
- composed of
docs/concepts/microfrontends.md
Outdated
| Micro frontends are a pattern where discrete UIs (frontends) are composed from | ||
| independent fragments that can be built and deployed separately by different | ||
| teams and loaded on-demand at runtime. Modular gives developers the opportunity | ||
| of implementing microfrontends through |
There was a problem hiding this comment.
Grammar:
- opportunity to implement
Consistency:
- Pick one of "microfrontends", "micro frontends" or "micro-frontends".
Content:
- Does modular give developers the opportunity to implement micro-frontends via ESM Views or is it that ESM Views can be used to create micro-frontend components via these views? There's a distinction to be made between the macro container and the micro frontends.
There was a problem hiding this comment.
Grammar, Consistency: Done
Content: I changed the sentence and made "ESM Views" the subject. Hope it's clearer now.
docs/concepts/microfrontends.md
Outdated
| teams and loaded on-demand at runtime. Modular gives developers the opportunity | ||
| of implementing microfrontends through | ||
| [ESM views](../building-apps/esm-views.md), which are applications built as ES | ||
| Modules, that can be served standalone or dynamically imported by an host |
docs/concepts/microfrontends.md
Outdated
| independent fragments that can be built and deployed separately by different | ||
| teams and loaded on-demand at runtime. Modular gives developers the opportunity | ||
| of implementing microfrontends through | ||
| [ESM views](../building-apps/esm-views.md), which are applications built as ES |
There was a problem hiding this comment.
nit: if we're capitalising the v in "View", we should capitalise the v in "ESM View", too.
There was a problem hiding this comment.
+1 for consistency. Done.
docs/concepts/microfrontends.md
Outdated
| of implementing microfrontends through | ||
| [ESM views](../building-apps/esm-views.md), which are applications built as ES | ||
| Modules, that can be served standalone or dynamically imported by an host | ||
| application. This is particularly useful when a View exports a default React |
There was a problem hiding this comment.
There's a difference between a "default React component" and "exports a React component as its default export" - use the latter.
However, I'd be more prescriptive with this definition. This is a teaching point about how to use modular; a simple example may help! Do developers have to export a React component as their default export? What if they want to export multiple things? I would start with a simple Hello World example and then mention how alternatives can be handled, if they can.
There was a problem hiding this comment.
There's a difference between a "default React component" and "exports a React component as its default export" - use the latter.
Do developers have to export a React component as their default export?
This doesn't apply anymore - I changed the sentence to be less specific.
Do developers have to export a React component as their default export? What if they want to export multiple things? I would start with a simple Hello World example and then mention how alternatives can be handled, if they can.
We're kinda still defining this; right now, the user can export whatever they want, but the trampoline expects a React component as default export. This means that if the user doesn't export a React component, the built ES Module will work perfectly as an ES Module, but serving the dist directory in standalone mode won't work. We still don't know what users will prefer / ask, so I wouldn't be too prescriptive here.
There was a problem hiding this comment.
(If you remember, we were discussing this point with with @steveukx and we said that in the future, depending on how users actually need to use esm-views, we might have a flag in the manifest configuration to trigger the creation of index + trampoline)
There was a problem hiding this comment.
Okay, so given that we are releasing this functionality in a somewhat beta state, we should make it clear that some of this is subject to change.
There was a problem hiding this comment.
Okay, so given that we are releasing this functionality in a somewhat beta state, we should make it clear that some of this is subject to change.
We have a technical description of how it works in the esm views.md file already: that reference file might / will change as we refine our technical approach. In this microfrontend.md file I'm trying to give a more bird's eye view of the topic; it doesn't matter how we import an esm-view; what matters is that we can import it at runtime without re-downloading its dependencies or create an iframe for it. I removed the "default export" part here as it's not so relevant and added it to esm views.md.
There was a problem hiding this comment.
We can call out that this is subject to change wherever you think is best. We don't want to surprise users who are depending on functionality that changes.
docs/building-apps/esm views.md
Outdated
|
|
||
| ## Customise bundling / rewriting strategy | ||
|
|
||
| By default, all external dependencies are rewritten to a CDN url and none are |
docs/building-apps/esm views.md
Outdated
| bundled. This logic can be controlled using two environment variables: | ||
|
|
||
| 1. `EXTERNAL_ALLOW_LIST` is a comma-separated string that specifies which | ||
| dependencies are allowed to be rewritten to CDN; if not specified, its |
docs/building-apps/esm views.md
Outdated
| dependencies are allowed to be rewritten to CDN; if not specified, its | ||
| default value is `**` ( -> all dependencies are rewritten) | ||
| 2. `EXTERNAL_BLOCK_LIST` is a comma-separated string that specifies which | ||
| dependencies are **not** allowed to be rewritten to CDN; if not specified its |
docs/building-apps/esm views.md
Outdated
|
|
||
| CSS imports pointing to an external package (for example: | ||
| [`import 'regular-table/dist/css/material.css'`](https://www.npmjs.com/package/regular-table) | ||
| ) will be rewritten to a CDN url as normal packages (for example, using skypack, |
There was a problem hiding this comment.
URL
Skypack
"as normal packages" is unclear. What's a "normal package"?
There was a problem hiding this comment.
Yeah, "as a normal package" doesn't make sense and it's not even needed - Removed it.
| [CSS Module scripts](https://web.dev/css-module-scripts/) and adding the script | ||
| to the | ||
| [adopted stylesheet](https://wicg.github.io/construct-stylesheets/#using-constructed-stylesheets). | ||
| This feature is experimental and feedback is appreciated. |
There was a problem hiding this comment.
New paragraph for this sentence
docs/building-apps/esm views.md
Outdated
| rewriting all of a subset of their imports to make use of a configurable esm CDN | ||
| (e.g. [Skypack](https://www.skypack.dev) or [esm.sh](https://esm.sh/)). This | ||
| allows users to implement the | ||
| [microfrontend pattern](../concepts/microfrontends.md), by creating an artefact |
There was a problem hiding this comment.
minor - if we're using american english, this should be artifact
Co-authored-by: Steve King <steve@mydev.co>
Co-authored-by: Steve King <steve@mydev.co>
5 participants
Docs for ESM Views