Skip to content

Conversation

georgyangelov
Copy link
Contributor

@georgyangelov georgyangelov commented Sep 4, 2024

Some important notes:

  • I've set it up with Vite instead of manual webpack configs. It's a good batteries-included tool that we've used in a lot of other projects and was pretty good. It does rely on rollup instead of webpack which I view as an upside to be honest, but if it starts creating issues integrating with the other projects (like scratch-gui) I'll replace it with a webpack-based setup.
  • I've set up TailwindCSS as a styling solution. Haven't provided a custom theme to it yet (it's using the defaults) - that will come when we starts having designs.
  • I created a scratch-web-shared package to host a Scratch component library and any other React utilities (hooks, etc.) that we may require
  • I haven't included any state management solutions intentionally - while Redux is regularly associated with React, I've found it to be unnecessary and inconvenient most of the time, especially for sites with not a lot of dynamic global state (which should be most, including this one). Usually state is page-based and can be hosted in page components. For instances of global state we can use useContext & useReducer where necessary.
  • I haven't included example pages yes, what I imagine for that is having views and components folders where views are pages and components are shared React components between those pages.
  • There is currently no router, that's one of the next things to set up (in a separate PR)
  • We plan on adding a OpenAPI-to-TS library next as well, to generate type-safe wrappers of fetch calls based on the backend services' documentation
  • I've left the default eslint setup generated by Vite. We'll have to go through and adapt existing rules from the other packages to be used here. They won't map 1:1 since we're using TS and some rules are different or don't make sense. Again thats for a separate PR.
  • I've added Prettier for automatic code formatting. Changed its config to use 4 spaces instead of 2 since I saw the other projects + the editorconfig says so. While regular JS conventions are for 2 spaces, I don't mind having 4, to make the code more consistent with the other projects.
  • Added a Husky script to check eslint rules and formatting at commit time.
  • I've set up the /config.js approach we discussed in one of the architecture calls - the idea being that we can build once and reuse those built artifacts for multiple environments (because env vars won't be hardcoded inside). To do that I wrote a Vite plugin to serve config.js on the dev server (getting env vars from .env) and inject the loading of config.js into the HTML for the bundle.

There will be more PRs with added boilerplate soon :)

@georgyangelov georgyangelov changed the title Gardens setup Setup scratch-gardens front-end project Sep 4, 2024
@georgyangelov georgyangelov changed the title Setup scratch-gardens front-end project Setup NextGen front-end project Sep 4, 2024
@georgyangelov georgyangelov marked this pull request as ready for review September 4, 2024 10:16
@georgyangelov georgyangelov changed the title Setup NextGen front-end project First pass at setting up NextGen front-end project Sep 4, 2024
Copy link
Contributor

@colbygk colbygk left a comment

Choose a reason for hiding this comment

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

LGTMSF


## Running Locally

1. You need to have the services in the `scratch-platform` monorepo running first.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note: if we are retaining fall backs to staging for locally running code, then it should be possible to run a minimalist amount of scratch-platform to support local dev, but if that's something we've already deviated from, we'll correct that dev ergonomics later (because a dev will get annoyed by it... and fix it!)

Copy link
Contributor Author

@georgyangelov georgyangelov Sep 5, 2024

Choose a reason for hiding this comment

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

My thinking was the set up is going to involve just:

  • Running a command in scratch-platform (which runs all services locally in docker) make run
  • Running a command here to start the front-end npm run dev

and that's it. If all you need to do to run locally is two commands, I don't feel we need to configure staging usage for dev ergonomics.
We will need to think it through before we open-source the frontend code, though, but I feel like we can think through that at a later stage. What do you think?

Copy link
Contributor

@colbygk colbygk Sep 5, 2024

Choose a reason for hiding this comment

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

That all reads as reasonable... Only, we've spent more than the last 10 years working this way, i.e. that defaults for all local development will go to staging unless otherwise directed by env vars to local resources... Mainly, my worry is that doing even trivial local dev will end up requiring a machine with > 16GB of RAM. (personally, I advocate for maxing out dev's RAM, but cost is a factor for us...)

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal preference for a public repository is that if someone outside of Scratch Foundation clones the repo and hits the "GO" button, they get as much non-Scratch-specific functionality as we can reasonably provide. That is, I'd prefer to bias toward self-contained local dev. Another positive of this approach is that it lets us avoid committing any of our real domain names (staging or prod) into the repo.

I also agree with what Colby's saying, though. I think my ideal would be that default behavior goes to localhost but it's easy to put entries into .env to use staging for some or all services. I think that's basically the direction this is heading in already, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, everything would be controllable through the .env file.

We can also have both .env.example and .env.example.staging at root-level and devs cloning the repo can pick which one to base their .env on? Does that seem reasonable in handling both cases? It would mean were committing real domain names, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. This looks like an SVG

"module": "ESNext",
"skipLibCheck": true,

/* Bundler mode */
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential change: /* Bundler mode activate */ (sorry, not sorry, Wonder Twins joke)

@cwillisf
Copy link
Contributor

cwillisf commented Sep 6, 2024

Some notes on your notes :)

  • I've set it up with Vite instead of manual webpack configs. It's a good batteries-included tool that we've used in a lot of other projects and was pretty good. It does rely on rollup instead of webpack which I view as an upside to be honest, but if it starts creating issues integrating with the other projects (like scratch-gui) I'll replace it with a webpack-based setup.

I have no objections to moving away from webpack -- actually, I might prefer moving -- as long as we can make everything work.

  • I created a scratch-web-shared package to host a Scratch component library and any other React utilities (hooks, etc.) that we may require

🎉

  • I haven't included any state management solutions intentionally - while Redux is regularly associated with React, I've found it to be unnecessary and inconvenient most of the time, especially for sites with not a lot of dynamic global state (which should be most, including this one). Usually state is page-based and can be hosted in page components. For instances of global state we can use useContext & useReducer where necessary.

100% OK by me. We have some pretty complex state in the editor, but I hope it's not necessary outside the editor.

  • I've left the default eslint setup generated by Vite. We'll have to go through and adapt existing rules from the other packages to be used here. They won't map 1:1 since we're using TS and some rules are different or don't make sense. Again thats for a separate PR.
  • I've added Prettier for automatic code formatting. Changed its config to use 4 spaces instead of 2 since I saw the other projects + the editorconfig says so. While regular JS conventions are for 2 spaces, I don't mind having 4, to make the code more consistent with the other projects.

I'm flexible on styling, and I don't feel the need to stick to what eslint-config-scratch demands. In fact, if we can use a set of styling rules that someone else maintains (maybe the Vite defaults, for example), I think that's better. You might notice that different editor modules use different major versions of eslint-config-scratch... :sadpanda:

Anyway, the point I'm getting at is: since our TS must use somewhat different rules, I think it's OK if our TS uses very different rules if it reduces our maintenance burden in the long term.

  • Added a Husky script to check eslint rules and formatting at commit time.
  • I've set up the /config.js approach we discussed in one of the architecture calls - the idea being that we can build once and reuse those built artifacts for multiple environments (because env vars won't be hardcoded inside). To do that I wrote a Vite plugin to serve config.js on the dev server (getting env vars from .env) and inject the loading of config.js into the HTML for the bundle.

👍

Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

This looks great so far! I'm excited!

I did have a thought I'd like to discuss, but it's not really part of the code review......

trailingComma: 'all',

plugins: [
require.resolve('@trivago/prettier-plugin-sort-imports'),
Copy link
Contributor

Choose a reason for hiding this comment

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

😎


## Running Locally

1. You need to have the services in the `scratch-platform` monorepo running first.
Copy link
Contributor

Choose a reason for hiding this comment

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

My personal preference for a public repository is that if someone outside of Scratch Foundation clones the repo and hits the "GO" button, they get as much non-Scratch-specific functionality as we can reasonably provide. That is, I'd prefer to bias toward self-contained local dev. Another positive of this approach is that it lets us avoid committing any of our real domain names (staging or prod) into the repo.

I also agree with what Colby's saying, though. I think my ideal would be that default behavior goes to localhost but it's easy to put entries into .env to use staging for some or all services. I think that's basically the direction this is heading in already, right?


Environment variables do not follow the regular convention where they are statically defined in the output JS bundle. Instead, they are served through a `config.js` file that is expected to be available on the root of the (sub)domain where the application is served. A plugin in [vite-config-js-plugin.ts] adds a reference to `/config.js?v=<random-string>` to be loaded before the JS bundle. The random string changes on each build and is there to bust any caches when a new version of the code is deployed.

The goal is to be able to deploy a build artifact (the JS/asset bundle) to multiple environments instead of having to rebuild for each one (as normally env variables are statically inserted into the bundle).
Copy link
Contributor

Choose a reason for hiding this comment

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

I REALLY like this. Not only for repeatability between staging & production, but also to ensure that we can locally debug exactly the code that's live.

@cwillisf
Copy link
Contributor

cwillisf commented Sep 6, 2024

So... I'm starting to have second thoughts on where this code should live. I'm sorry!

Basically, I'd like everything in scratch-editor to be useful to someone who wants to build a fork of the Scratch editor. Specifically, if someone outside the Scratch Foundation clones the repo, I think everything in it should be useful to them. This idea is why I advocated for leaving scratch-www out of the monorepo, but I guess I forgot that argument when we talked about where to put the front-end NGP work.

On the other hand, I really like the idea of a future where scratch-gui can share web components with community pages and maybe even moderator/admin features. That suggests leaving those shared basic components in the monorepo. However, that means their active development would be split between two repositories.

So, I can think of two reasonable proposals:

  1. scratch-web and scratch-shared-web remain in the scratch-editor repository and we just decide that it's fine
  2. scratch-web and scratch-shared-web move to a repository that will always be private, and some day we might move scratch-shared-web back to this repo (once scratch-gui wants to use it... maybe never)

Thoughts? Alternative proposals?

@georgyangelov
Copy link
Contributor Author

@cwillisf Those are all excellent points. I was assuming we wanted to have the front-end open-source, but hadn't thought too much about it.

It would definitely be better for development if we have both the front-end and backend in the same repository - in terms of releases, version management, CI & synchronized code changes as well. This implies that scratch-web is private, which is why I hadn't brought it up.

In terms of scratch-shared-web, I like your second proposal - we would have time to flesh it out and stabilize it during NextGen development. Once we start wanting to use it for scratch-gui, it would have enough that we don't need to iterate on it quite as much and decoupling from the other monorepo (and publishing through npm) would be easier.

@colbygk, @chrisauer, how do you feel about what's being discussed here? I was also thinking that it would be a good idea to have another monorepo with backend+frontend (instead of scratch-platform) for the NextGen work so that we don't have services for both the current Scratch (uploader, sortinghat) and the NextGen in the same place. Would make it cleaner to work with (no READMEs with "if working with current platform do this / if working with NextGen do that") and it would be decoupled in terms of CI as well.

@colbygk
Copy link
Contributor

colbygk commented Sep 9, 2024

@colbygk, @chrisauer, how do you feel about what's being discussed here? I was also thinking that it would be a good idea to have another monorepo with backend+frontend (instead of scratch-platform) for the NextGen work so that we don't have services for both the current Scratch (uploader, sortinghat) and the NextGen in the same place. Would make it cleaner to work with (no READMEs with "if working with current platform do this / if working with NextGen do that") and it would be decoupled in terms of CI as well.

I think it would be better to not have the "web" aspect of Scratch be open source, and leave the editor fully open source which would make our open source stance more specific, and help us avoid the community lawyering us about having the backend not open source, while all of the frontend is open source.

Long term, I think it's on mission to have an open source editor while the web site frontend, is not necessarily something we're using as part of a mission to have external third parties have the ability to lift the entirety of scratch-www.

@cwillisf
Copy link
Contributor

cwillisf commented Sep 9, 2024

Another ingredient that occurred to me while I was thinking about this over the weekend: we've talked about wanting the Scratch community website to be powered by a CMS so that less-technical folks can make updates to the content. That's far enough away that we can't block current work on it, but it's something to keep in mind in terms of long-term direction of travel.

In my eyes, having our website open-source is a nice-to-have so that folks can help us out, but the value is mostly to us: other folks won't learn much from our website, and the value of forking it is even lower. With that in mind, I'm OK with making our website private: we're not taking much value away. I also agree with @colbygk that this makes our open-source stance clearer.

Combining those two things with the discussion above, I agree with the plan @georgyangelov described. I'd extend it this way:

  1. Short term: optimize for rapid progress on the next-gen platform
    • Put the next-gen front-end work in the same repository as the next-gen back-end work
    • Build the "shared" web components in that repository as well
      • ...as scratch-web-shared or directly in the next-gen web project? I'm flexible on this.
  2. Medium term: if and when we want to use those shared web components in the editor GUI, we can discuss moving scratch-web-shared into scratch-editor (which will probably be public by then).
    • Maybe we move the whole scratch-web-shared module (if there is one!)
    • Maybe we move individual components
  3. Long term: if and when we want to use a CMS for our community website, we'll have s'more discussions
    • If our CMS includes front-end rendering, the concept of shared React components may go away, and maybe all of the relevant UI components will move here
    • If we use a headless CMS with a React front-end, we may want to continue sharing components as described in the "Medium term"
    • Regardless, this stuff is far enough beyond the horizon that we can't yet make solid plans for it

Does that work for everyone?

@cwillisf
Copy link
Contributor

The Scratch engineering team discussed this today, and everyone is on board with the plan I described above.

@georgyangelov
Copy link
Contributor Author

georgyangelov commented Sep 11, 2024

@cwillisf, @colbygk sounds good, thank you for reviewing and confirming with everyone else as well. Closing this PR now, and will open in a different repository soon

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