-
Notifications
You must be signed in to change notification settings - Fork 5
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
remix integration initial commit #105
Conversation
✅ Deploy Preview for signup-api canceled.
|
✅ Deploy Preview for dmno ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
v3_relativeSplatPath: true, | ||
v3_throwAbortReason: true, | ||
}, | ||
presets: [dmnoRemixPreset() as any], |
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.
one slight bummer, currently we have to add the preset and the vite plugin, although this is only necessary to inject the public+dynamic config api route. Alternatively we could have people just add the route file more explicitly.
"moduleName": "@dmno/remix-integration", | ||
"importVars": [ "dmnoRemixVitePlugin", "dmnoRemixPreset" ], | ||
} ], | ||
"updates": [ { |
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.
this does not currently include the additional "remix preset" that needs to be inserted
`, | ||
|
||
|
||
dmnoInjectionResult.serviceSettings.preventClientLeaks ? ` |
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.
most of this code is copied from the next integration. so we can try to clean this up and extract it for reuse.
// TODO: ideally we would use a virtual module but remix seems to be making some assumptions about a real file existing | ||
|
||
// instead awkwardly we need to use a relative path and need it based on the build output dir which is not available here | ||
const relativeRoutePath = relative(buildDir, `${__dirname}/public-dynamic-config-api-route.js`); |
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.
this is dumb - going to file some issues w/ remix, so we can use a virtual module instead.
4bf7837
to
71fefa1
Compare
const objectSrc = originalSrc.substring(nodeToUpdate.start, nodeToUpdate.end); | ||
const isMultiLine = objectSrc.includes('\n'); | ||
// handle multi-line objects | ||
if (isMultiLine) { |
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.
fixing multi-line handling
} | ||
} | ||
// this looks for a function call by name | ||
} else if (singleUpdate.symbol.endsWith('()')) { |
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.
previously we only handled modifying the default export, now we can also just look for a function call by name. This is needed to mess with the remix()
vite plugin options
return; | ||
} | ||
if ((globalThis as any)._dmnoHttpInterceptor) { | ||
// console.log('disposing existing interceptor'); | ||
(globalThis as any)._dmnoHttpInterceptor.dispose(); |
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.
this is what was broken in netlify, but I suspect it might be breaking something else (like nextjs + edge?). should be uncovered when we get the full test suite set up.
} | ||
// for the edge CJS build | ||
// the node imports of zlib and http dont get fully tree-shaken away, so we'll just remove the lingering dead code | ||
if (process.env.DMNO_EDGE_COMPAT) { |
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'd love to get rid of these additional adjustments, but it seems the other options I get with esbuild/tsup cant quite do it.
@@ -21,6 +21,14 @@ Legend: | |||
- 🗓️ Planned | |||
- ❌ Not Supported | |||
|
|||
### [Remix](/docs/integrations/remix/) |
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.
support is looking pretty good across the board, so I think we may want to re-think this page
@@ -168,41 +164,6 @@ export function dmnoNextConfigPlugin(dmnoOptions?: DmnoPluginOptions) { | |||
},` : '', | |||
'});', | |||
|
|||
|
|||
// here we patch the global Response to pipe the body through the leak scanner |
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.
was able to move all of this into the core and into the main injector code 🎉
plus additional related refactoring of other integrations
Remix integration is coming together nicely! Needs a bit of cleanup and add docs, but it's all functional.