-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-3009: Convert snooty-frontend to use ESM style imports at build time #963
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Leaving some comments to fix up some issues I saw related to the Gatsby Cloud preview plugin.
import { parser } from 'stream-json/jsonl/Parser'; | ||
import { sourceNodes } from './other-things-to-source.mjs'; | ||
import parser from 'stream-json/jsonl/Parser.js'; | ||
import { sourceNodes as sourceNodesLocal } from './other-things-to-source.mjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what the best alias for this function would be, maybe sourceNodesOld
? idk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think sourceNodesLocal
works. Or sourceMiscNodes
? We can probably even change the exported function's name directly instead of using an alias, to make whichever name we go with clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment, but it shouldn't be a blocker either way.
Not sure what the plan is for this PR (whether we want to merge immediately, or wait to merge along with the fixed tests), but this LGTM
import { parser } from 'stream-json/jsonl/Parser'; | ||
import { sourceNodes } from './other-things-to-source.mjs'; | ||
import parser from 'stream-json/jsonl/Parser.js'; | ||
import { sourceNodes as sourceNodesLocal } from './other-things-to-source.mjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think sourceNodesLocal
works. Or sourceMiscNodes
? We can probably even change the exported function's name directly instead of using an alias, to make whichever name we go with clearer.
@mayaraman19 I noticed when running [webpack.cache.PackFileCacheStrategy] Caching failed for pack: Error: Can't resolve '/Users/caesarbell/10gen/docs-platform/snooty/plugins/gatsby-source-snooty-prod/gatsby-node.js' in '/Users/caesarbell/10gen/docs-platform/snooty'
<w> while resolving '/Users/caesarbell/10gen/docs-platform/snooty/plugins/gatsby-source-snooty-prod/gatsby-node.js' in /Users/caesarbell/10gen/docs-platform/snooty as file @rayangler is this something we should look into? |
@caesarbell That caching issue was supposed to be resolved after a specific Gatsby version (see: comment). Did you pull down the latest changes from this branch and reinstall dependencies? |
Stories/Links:
DOP-3009
Current Behavior:
Currently, the build-step code uses
require()
(CommonJS) statements instead of ESM-styleimport
statements. This PR fixes that. There are a couple notes/issues, however:"type" : "module"
topackage.json
seems to introduce a Gatsby caching error that can seemingly only be solved by editing files withinnode_modules
. In order to avoid this, I did not add"type" : "module"
but instead changed any files within the dependency tree ofgatsby-config.mjs
andgatsby-node.mjs
to also end with the extension.mjs
.npm run build
that seems to be related to caching:In addition, Jest does not like when any of its tests have a file that ends with
.mjs
in their dependency tree, so a lot of tests are broken now and fail. These have been excluded, and a ticket will be opened to resolve this. I also don't think this ticket should be merged until we find a way to resolve the broken tests.Staging Links:
Staging link (but doesn't really matter tbh)
Notes: