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

Setup Monorepo #805

Merged
merged 727 commits into from
Jan 29, 2021
Merged

Setup Monorepo #805

merged 727 commits into from
Jan 29, 2021

Conversation

xg-wang
Copy link
Member

@xg-wang xg-wang commented Nov 28, 2020

What it does

This PR sets up the monorepo for several repositories under ember-fastboot org, and internal test packages for easier testing across several repos:

  • packages/fastboot: a low-level tool for anything relying on rendering Ember app in node environment (e.g., custom fastboot-server, prember)
  • packages/ember-cli-fastboot: required for all SSR ember app
  • packages/fastboot-express-middleware: low-level tool for building custom fastboot-server
  • packages/fastboot-app-server: sane default for serving production app
  • test-packages/*: internal ember apps for acceptance & e2e testing, not published

All packages to be published are bumped to 3.1.2, which is the largest among the packages' versions.

Also converting several fastboot repo tests that rely on ember-cli-fastboot for building ember app into test-packages/integration-tests/test/basic-test.js.

ember-cli-fastboot is already using release-it-yarn-workspace to release ember-cli-fastboot; we can continue to use the release flow to release others.

Context

ember-fastboot org has multiple packages useful for server-side rendered ember apps.
Although these packages can be used together with different versions combination (e.g., https://github.com/ember-fastboot/fastboot/blob/v3.1.2/src/fastboot-schema.js is explicitly designed for compatibility), it is usually desired to keep them in sync from maintainers' perspective and use a combination of versions known good from users'.

Currently, when bumping one package, several PRs need to cascade for other packages; it becomes even trickier when a fastboot contributor needs to make a change or write a test that interacts with different packages.

To merge these related packages into a monorepo, we can make maintaining and contributing to fastboot much easier.

The cons for doing monorepo is losing the flexibility to release different versions for each package and increasing the coupling, but they are not necessarily bad IMO.

Checklist

The monorepo setup needs to ensure a couple of things:

  • link packages in dev/test
  • ci pass
  • move some fastboot tests to integration-tests for demonstration (basic-test.js)
  • update packages/*/readme.md
  • release setup
  • keep all the git history of the different packages
  • (need to verify after merge) make sure the list of contributors on GitHub is a superset

Next steps

  • After this PR is merged, move issue/pr from other repos then archive (We can only move manually/individually now 😥)
  • Identify and fix any incompatibilities
  • Identify and document remaining duplicated tests from packages, then move them to test-packages
  • greenkeeper -> dependapot
  • unify linting
  • fix ci (unskip windows 😥)
  • drop node 13 (fastboot is specifying "node": "10.* || >=12" whereas others are specifying "node": "10.* || 12.* || >= 14"
  • Merge https://github.com/ember-fastboot/fastboot-website into the monorepo for live demo, prod smoke testing and documentation.

rondale-sc and others added 30 commits February 23, 2018 15:30
This is part of an arching plan to introduce Glimmer's
rehydration/serializtion modes to Ember proper.

There are 4 PR's that are all interwoven, of which, this is one.

In order of need to land they are:

Glimmer.js:
glimmerjs/glimmer-vm#783 (comment)

This resolves a rather intimate API problem.  Glimmer-vm expects a very
specific comment node to exist to know whether or not the rehydration
element builder can do it's job properly.  If it is not found on the
first node from `rootElement` it throws.

In fastboot however we are certain that there will already be existant
elements in the way that will happen before rendered content.

This PR just iterates through the nodes until it finds the expected
comment node.  And only throws if it never finds one.

---

Ember.js:
emberjs/ember.js#16227

This PR modifies the `visit` API to allow a private _renderMode to be
set to either serialize or rehydrate.  In SSR environments the
assumption (as it is here in this fastboot PR) is that we'll set the
visit API to serialize mode which ensures glimmer-vm's serialize element
builder is used to build the API.

The serialize element builder ensures that we have the necessary fidelty
to rehydrate correctly and is mandatory input for rehydration.

---

Fastboot:
ember-fastboot/fastboot#185

This allows enviroment variable to set _renderMode to be used in Visit
API.  Fastboot must send content to browser made with the serialization
element builder to ensure rehydration can be sucessful.

---

EmberCLI Fastboot:
ember-fastboot#580

Finally this does the fun part of disabling the current
clear-double-render instance-initializer

We first check to ensure we are in a non-fastboot environment.  Then we
ensure that we can find the expected comment node from glimmer-vm's
serialize element builder.  This ensures that this change will only
effect peoeple who use ember-cli-fastboot with the serialized output
from the currently experimental fastboot setup

Then we ensure `ApplicationInstance#_bootSync` specifies the rehydrate
_renderMode.

This is done in `_bootSync` this way because visit is currently not used
to boot ember applications.  And we must instead set bootOptions this
way instead.

We also remove the markings for `fastboot-body-start` and
`fastboot-body-end` to ensure clear-double render instance-initializer
is never called.
…tion-serialization-from-glimmer

Rehydration
Took me a bit to figure out if this was possible.
When accessing a header, fastboot [will lowercase the string](https://github.com/ember-fastboot/fastboot/blob/master/src/fastboot-headers.js#L51-L57). However, when adding headers to the FastBootHeaders, the casing [is not changed](https://github.com/ember-fastboot/fastboot/blob/master/src/fastboot-headers.js#L20).

This PR normalizes all headers as lower case strings, so `X-Headers` will be accessible on the `fastboot.request.headers` object.
* issue ember-fastboot#193
* Added unit tests to check domContents.body includes boundary script tag
…te_body_in_domContents

Moved the script tag fastboot-body-start boundary in _finalizeHTML method
funtion -> function
* 💥 remove v4
* add v8 and v10
…-sync

Remove usage of deprecated exists-sync
# Conflicts:
#	packages/fastboot-express-middleware/package.json

run tests in ci

skip legacy mocha tests

skip fastboot ci on windows

fastboot ci test is not working correctly on windows
port fastboot test into integration-tests package
@xg-wang
Copy link
Member Author

xg-wang commented Dec 11, 2020

I squashed the commits, this commit contains the new test setup 5451050 (#805)

@xg-wang
Copy link
Member Author

xg-wang commented Jan 6, 2021

@rwjblue (friendly ping) can I get a review on this PR?

README.md Show resolved Hide resolved
Copy link
Member

@kiwiupover kiwiupover left a comment

Choose a reason for hiding this comment

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

@xg-wang I have left a few comments describing which test packages the test should live in.

test-packages/custom-sandbox-app/README.md Show resolved Hide resolved
test-packages/hot-swap-app/README.md Outdated Show resolved Hide resolved
packages/fastboot/test/fastboot-test.js Outdated Show resolved Hide resolved
packages/fastboot/test/fastboot-test.js Outdated Show resolved Hide resolved
packages/fastboot/test/fastboot-test.js Outdated Show resolved Hide resolved
packages/fastboot/test/fastboot-test.js Outdated Show resolved Hide resolved
@kiwiupover
Copy link
Member

@xg-wang the first few of your commits show a blank diff any idea why?

0f5b00b

@xg-wang
Copy link
Member Author

xg-wang commented Jan 17, 2021

@kiwiupover

this test could be added to the basic-app test-package.
Thanks for pointing out! I assume you are saying the tests I moved from fastboot/test/fastboot-test.js to test-packages/integration-tests/test/basic-test.js, where fixture basic-app is used.
Sorry I forgot to mention explicitly the migration path, which I'm thinking about to move basic-app/test/*.js to test-packages/integration-tests/test.
The thought here is we use test-packages/basic-app purely as a fixture app, and write tests under test-packages/test/*.js. I think the advantages are:

  • we can test against multiple dists
  • we shift the mental model about integration test boundaries. In basic-app/test we serve the app and test HTML response, in new setup we use fastboot.visit directly, and test middleware (currently packages/fastboot-express-middleware/test/middleware-test.js in another file (I haven't created yet). In new middleware integration test we don't need to test each test app, we just need to use some test app (basic-app should be enough most time) to cover middleware behavior
  • Not starting a server for every test file should execute faster

I prefer to move test-packages/basic-app/test/*-test.js to test-packages/integration-tests/test/<maybe-new-name>-test.js. It would be a simple move change at first, and change starting server to use fastboot.visit when we have time (hope we will clean up the TODO 😄), as we migrate more old tests in ember-cli-fastboot we don't need to start the server. I couldn't think of cons for doing so, except the churn of moving them.

Any thoughts?


For the PR description TODO item

Identify and document remaining duplicated tests from packages, then move them to test-packages

It's my miss I have not written more ideas, and I think this is important for deciding where to have the tests in this PR.

If you look at existing tests in each packages, all fastboot, fastboot-express-middleware, fastboot-app-server use fixtures and implicitly rely on each other packages to build/serve the app. I think we need to move these integration pieces into test-packages/integration-tests/test/*-test.js, or figure out the boundary and test the unit behaviors in package's own test folder. There need to be more thoughts go into each PR when doing so.

@xg-wang
Copy link
Member Author

xg-wang commented Jan 17, 2021

@xg-wang the first few of your commits show a blank diff any idea why?

0f5b00b

@kiwiupover that's in the repo moved into the monorepo (for example fastboot), I committed a pre-merge commit to move everything to packages/ so the later merge has no conflicts. GitHub may have some issue displaying the diff. I used the following script:

cd fastboot
mkdir -p ../packages/fastboot; mv .* * $_
mv ../packages .
mv packages/fastboot/.git .
git commit -m 'Monorepo setup: move to packages/fastboot'
# repeat for other repos
git remote add local/fastboot file://$HOME/Code/ember-fastboot/fastboot
git fetch local/fastboot
git merge local/fastboot/master --allow-unrelated-histories

GitHub may have some issue displaying them, you can see it's the file move by doing git show 0f5b00b8442bcc6c08fc2f69d4cfdb710e11eb3a

add integration-test in ci
rename lib to test-helper
@xg-wang
Copy link
Member Author

xg-wang commented Jan 23, 2021

Paired with @kiwiupover, we walked through the PR and listed out the things needed:

Must:

  • add integration-test in ci
  • rename lib to test-helper
  • merge into master, ask Rob to release 3.1.3-beta, confirm npm packages work

After merge:

  • check contributor list is updated
  • update other packages, remove files, update readme about redirect link
  • over next week: transfer issues, close or re-open PRs

In follow-up PRs:

  • update buildDist helper to accept prod build flag
  • refactor lib/clean-dists.js to walk fs structure
  • consolidate
    • consolidate hot-swap-app and custom-sandbox-app to sample-app-custom-fastboot
    • rename basic-app -> sample-app
    • Write testing-readme instruction for few things
      • packages/ have unit tests
      • integration tests (e.g., basic-app/test/) move to integration-tests
      • Identify duplicate tests from packages/*
      • In the future, think about basic-app/test/ contain app test code using ember-cli-fastboot-testing, to test fastboot provided test-helpers for app developers are working correctly

@@ -4,7 +4,7 @@
"version": "3.1.2",
"repository": "",
"scripts": {
"test": "node lib/clean-dists.js && mocha"
"test": "node helpers/clean-dists.js && mocha"
Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

@kiwiupover kiwiupover left a comment

Choose a reason for hiding this comment

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

@xg-wang Great work Thomas!

@kiwiupover
Copy link
Member

@rwjblue this is ready to be merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.