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

[PR] Creating nextra with auth #2

Merged
merged 75 commits into from
Jul 16, 2024
Merged

[PR] Creating nextra with auth #2

merged 75 commits into from
Jul 16, 2024

Conversation

LuchoTurtle
Copy link
Member

addresses #1

This PR adds a new Nextra project that has been modified to accommodate the requirements of #1 .

Authentication has been added successfully, and routes have been protected.

Going to check the Search requirement, although I feel like this will entail using a custom-theme, which is a heavy workload. Will look into it 🔎

@LuchoTurtle LuchoTurtle added documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person epic A feature idea that is large enough to require a sprint (5 days) or more and has smaller sub-issues. labels Jun 21, 2024
@LuchoTurtle LuchoTurtle self-assigned this Jun 21, 2024
@LuchoTurtle
Copy link
Member Author

I'm trying to add E2E testing with Playwright and running it with a simple one already works.
But I want to get the coverage out of the test reports and I'm having a bit of trouble getting there.

Following microsoft/playwright#7030 (comment), and subsequently https://github.com/edumserrano/playwright-adventures/blob/main/demos/code-coverage-with-monocart-reporter/README.md
got me to the point that I do get the coverage but I can't find a way to only include the relevant directories (right now, it's calculating node_modules, no way josé).

@nelsonic
Copy link
Member

Looking good. If you're still stuck with ignoring the node_modules and other non-relevant files, LMK. 👌

@LuchoTurtle
Copy link
Member Author

E2E testing added with Playwright and added coverage with monocart-reporter. The coverage unfortunately is only using the built files and not the source maps. But since these are E2E tests, I'm not much concerned with coverage from them. What matters is that they pass.

I'm thinking of getting coverage working for unit tests with something like vitest or jest, though.

@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Jul 7, 2024
@nelsonic
Copy link
Member

nelsonic commented Jul 8, 2024

Nice one. I will review this evening after work-work & kids are asleep. 👌

@nelsonic
Copy link
Member

nelsonic commented Jul 8, 2024

On this branch, ran:

pnpm i 
pnpm run dev

Get:

image

So filling in the gaps for running the finished version of the project. 🧑‍💻

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Jul 8, 2024

That's weird. I've deleted both .next and node_modules on both the root and inside theme directory and ran pnpm i and pnpm run dev and it works on my end ("but it works on my computer!" 🖥️). It does build correctly inside the Github Action, so I'm interested in how you got that error 🤔

Besides that, you're going to need to complete .env.local-example (and rename it to .env.local, as per the docs). You ought to follow the docs to create a Github App and secret to use in the .env file.

@nelsonic
Copy link
Member

nelsonic commented Jul 9, 2024

@LuchoTurtle I'm just pointing out that there are no instructions in the README.md for running the finished project.
You just dive into creating it from scratch which goes against Habit #2: "Begin with the end in mind" i.e. you don't give people the clear picture of the finished project so they know what they are working towards.

For an example of this, see: "First Run the Finished App" in the Phoenix Chat Example:
https://github.com/dwyl/phoenix-chat-example?tab=readme-ov-file#first-run-the-finished-app

@nelsonic nelsonic closed this Jul 9, 2024
@nelsonic nelsonic reopened this Jul 9, 2024
@nelsonic
Copy link
Member

image image

Really don't like how nextra prefixes all Tailwind classes with nx-
image
totally pointless extra typing 🤦‍♂️

Given the choice, I definitely wouldn't use Nextra for a docs project there are so many steps, config files and unnecessary complexity for a suboptimal result. The theme/src/components/flexsearch.tsx is ~300loc of undocumented code which in turn invokes theme/src/components/search.tsx ~300loc of undocumented code. 🤦‍♂️

It's clear that this is meant to be run "as is" and not customised very much.

Constantly seeing the Error that it cannot validate the _meta.json is infuriating:

[nextra-theme-docs] Error validating _meta.json file for "reference_api" property.

Unrecognized key(s) in object: 'private'
[nextra-theme-docs] Error validating _meta.json file for "mega_private" property.

Unrecognized key(s) in object: 'private'
 GET /_next/data/development/reference_api/person.json 200 in 814ms

And taking 814ms to render a page that hasn't changed. 🐌 🤷‍♂️

I clearly need to get better at giving precise requirements. 💭
this is why we end up with so many micro-managers. .. 😢

All we wanted was a way to streamline the process of writing docs from docusaurus ...
Instead we get 30kloc of PR that very few people will be able to understand much less maintain
and more complexity for writing docs.
No wonder everyone is using notion ... why would anyone waste their time on something even more complex?! 🤷‍♂️

I'm definitely not "happy" with this.
If I could use one emoji to describe how I feel it's:

😿

@LuchoTurtle I think given the circumstances and incomplete instructions you've done a good job.

The tutorial.md (split out of README.md) is 42 pages:
nextra-demo-doc-42-pages

How many people do you think will read/follow it? 💭

I'm frustrated at the complexity of Nextra ... as I'm sure you must have been for the past few weeks. ⏳
Definitely not a reflection on you; you didn't choose this tech/tool you just rolled with it! 👌

Anyway, merging. :shipit:

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Thanks @LuchoTurtle 👌

@nelsonic nelsonic merged commit 59a5106 into main Jul 16, 2024
3 checks passed
@nelsonic nelsonic deleted the learn-nextra-#1 branch July 16, 2024 17:26
@LuchoTurtle
Copy link
Member Author

Just adding a few comments:

  • I fully agree that Nextra is meant to be used as-is, given the limited customization options. The work done in this PR was meant to allow the person to have easy control of private routes dynamically by embedding this functionality within the _meta.json (hence why you get the warnings from Nextra saying they don't recognize my added private property - it is foreign to it).
  • taking 814ms to render a page that hasn't changed is fair when running in development mode. If the site is built and then npm start using the built site in production mode, it is much faster. Though, I agree that having these compile times in the development cycle can be cumbersome.
  • "I clearly need to get better at giving precise requirements. 💭 this is why we end up with so many micro-managers. .. 😢" can't help but think this a politically correct way of you saying I misinterpreted the requirements. This can be a fair assertion, since I can understand your frustration on dealing with a 30k-line PR like this where you wanted to KISS. Though I may be wrong! :p
  • the code we will be using in /docs for A will be much more pleasant to work with. I learnt a lot with Nextra and its nuances in this PR, so I'm positive the monorepo approach we're having at A's /docs will be much more manageable (even if it's not optimal having 3 projects running as a standalone unit, I agree with you on that).

Thanks for taking the time to review this, it's mighty appreciated! 🙏

@nelsonic
Copy link
Member

The recipient of the message should never be responsible for interpreting the message.
That's how we end up with legal "interpretations" and "grey areas" which waste time.
The onus is always on the person transmitting the message - this case me - to ensure the message is as clear as possible. I will be better. 🔰 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality epic A feature idea that is large enough to require a sprint (5 days) or more and has smaller sub-issues.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants