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

Bug/issue 386 graph not matching filesystem order #449

Merged

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Dec 18, 2020

Related Issue

resolves #386 (and incorporates #455 / #331 somewhat) , as I have noticed it is happening in my personal project still, and fairly consistently- thegreenhouseio/www.thegreenhouse.io#174 (comment)

Summary of Changes

  1. Made walkDirectory logic synchronous, which makes this issue go away at least (async programming is hard 😞 )
  2. Bulked up the Nested Directory test case with a bunch of fixtures to ensure this gets solid test coverage
  3. Cleaned up page object pushed into the graph just to prune properties that aren't so relevant anymore
  4. Made sure route is normalized and "safe" to use and always end with /, solves routes with and without trailing forward slash causes route "misses" #331

As an observation, I think as part of the data work, we should avoid remapping properties in graph.json and GraphQL. For example in graph.json it's label, in GraphQL it's id. We should just mirror them unless there's a reason they can't, IMO. Something I can do as part of #278 and its accompanying documentation updates.

Nice To Have

  1. Try and get it working asynchronously, though I don't want that to be show stopper, as I am happy to make a refactoring issue to track finding an asynchronous solution (and at least have a working solution until then)
  2. refactor walkDirectory to not rely on context, so it can move out of the scope of module.exports

edit: tracking both of the above in Trello for post 1.0 efforts / optimizations

@thescientist13 thescientist13 added bug Something isn't working P0 Critical issue that should get addressed ASAP CLI Data Data and GraphQL labels Dec 18, 2020
@@ -33,7 +33,7 @@ class HTMLTransform extends TransformInterface {
let body = await getAppTemplate(barePath, this.workspace);
body = await getAppTemplateScripts(body, this.workspace);
body = getUserScripts(body, this.workspace);
body = getMetaContent(url, this.config, body);
body = getMetaContent(url.replace('index.html', ''), this.config, body);
Copy link
Member Author

Choose a reason for hiding this comment

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

this was sending URLs w/ index.html in the url so this strips that to keep nice clean URLs, e.g.

www.example.com/about/index.html

is now

www.example.com/about/


although this makes more sense to do in getMetaContent?

Copy link
Member Author

Choose a reason for hiding this comment

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

edit x2: actually, looks like I don't need it there, the tests pass without it, so I must have fixed the issue that caused this. 😎

@thescientist13 thescientist13 force-pushed the bug/issue-386-graph-not-matching-filesystem-order branch from b2be2da to 70e9dc4 Compare December 28, 2020 18:26
@thescientist13 thescientist13 removed their assignment Dec 28, 2020
@thescientist13 thescientist13 added the documentation Greenwood specific docs label Dec 28, 2020
@thescientist13 thescientist13 linked an issue Dec 30, 2020 that may be closed by this pull request
5 tasks
@thescientist13 thescientist13 merged commit 1eade4f into release/0.10.0 Dec 30, 2020
@thescientist13 thescientist13 deleted the bug/issue-386-graph-not-matching-filesystem-order branch December 30, 2020 17:41
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* init refactor with synchronous approach working

* robust nested directory output testing

* formatting

* robust nested directory output testing

* graph page object clean up

* graph lifecycle refactor:

* revert transform changes

* fix #455

* fix routing logic

* fix incorrect output when pages have index in the name
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* init refactor with synchronous approach working

* robust nested directory output testing

* formatting

* robust nested directory output testing

* graph page object clean up

* graph lifecycle refactor:

* revert transform changes

* fix #455

* fix routing logic

* fix incorrect output when pages have index in the name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLI Data Data and GraphQL documentation Greenwood specific docs help wanted Extra attention is needed P0 Critical issue that should get addressed ASAP
Projects
None yet
1 participant