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

DOP-3009: Convert snooty-frontend to use ESM style imports at build time #963

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

mayaraman19
Copy link
Collaborator

@mayaraman19 mayaraman19 commented Nov 28, 2023

Stories/Links:

DOP-3009

Current Behavior:

Currently, the build-step code uses require() (CommonJS) statements instead of ESM-style import statements. This PR fixes that. There are a couple notes/issues, however:

  • adding "type" : "module" to package.json seems to introduce a Gatsby caching error that can seemingly only be solved by editing files within node_modules. In order to avoid this, I did not add "type" : "module" but instead changed any files within the dependency tree of gatsby-config.mjs and gatsby-node.mjs to also end with the extension .mjs.
  • there is now a warning that i'm not sure how to fix when running npm run build that seems to be related to caching:
[webpack.cache.PackFileCacheStrategy] Caching failed for pack: Error: Can't resolve '/Users/maya/Desktop/snooty/plugins/gatsby-source-snooty-prod/gatsby-node.js' in '/Users/maya/Desktop/snooty' 
<w> while resolving '/Users/maya/Desktop/snooty/plugins/gatsby-source-snooty-prod/gatsby-node.js' in /Users/maya/Desktop/snooty as file

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:

@mayaraman19 mayaraman19 marked this pull request as ready for review December 8, 2023 20:41
@mayaraman19 mayaraman19 mentioned this pull request Dec 11, 2023
Copy link
Collaborator

@rayangler rayangler left a 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.

plugins/gatsby-source-snooty-preview/gatsby-node.mjs Outdated Show resolved Hide resolved
plugins/gatsby-source-snooty-preview/gatsby-node.mjs Outdated Show resolved Hide resolved
plugins/gatsby-source-snooty-preview/gatsby-node.mjs Outdated Show resolved Hide resolved
plugins/gatsby-source-snooty-preview/gatsby-node.mjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
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';
Copy link
Collaborator Author

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

Copy link
Collaborator

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.

Copy link
Collaborator

@rayangler rayangler left a 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';
Copy link
Collaborator

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.

@caesarbell
Copy link
Collaborator

@mayaraman19 I noticed when running npm run develop we are getting the following error

[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?

@rayangler
Copy link
Collaborator

@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?

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

Successfully merging this pull request may close these issues.

3 participants