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

[RFC][1/2] Change .oss imports from relative to absolute + lint rule disallowing relative import of .oss modules + remove InjectedComponentContext. #23417

Merged
merged 9 commits into from
Aug 6, 2024

Conversation

salazarm
Copy link
Contributor

@salazarm salazarm commented Aug 5, 2024

Summary & Motivation

We are moving from a run time solution for injecting dependencies into OSS to a compile/build time solution.
The way our solution works is by using typescript paths in order to override .oss.tsx files with their .cloud.tsx counterpart of the same name. However, typescript paths doesn't work with relative paths, only absolute paths, so to make sure our .oss modules are imported as absolute paths I added a lint rule with an auto-fixer (which I used to update all these imports).

How I Tested These Changes

yarn lint --fix
yarn ts

Copy link

github-actions bot commented Aug 5, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-ocp1kfmra-elementl.vercel.app
https://salazarm-compose-cloud-oss.core-storybook.dagster-docs.io

Built with commit d1fa335.
This pull request is being automatically deployed with vercel-action

@salazarm salazarm requested review from bengotow and hellendag August 5, 2024 19:17
@salazarm salazarm marked this pull request as ready for review August 5, 2024 19:17
Copy link
Collaborator

@bengotow bengotow left a comment

Choose a reason for hiding this comment

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

This looks great! Overall I'm a big fan of trying this out for a week or two and iterating on it as we go 🙌

@@ -78,6 +78,7 @@ module.exports = {
'\\.(css|less)$': 'identity-obj-proxy',
'^worker-loader(.*)/workers/(.*)$': '<rootDir>/jest/mocks/$2',
'^@dagster-io/ui-components$': '<rootDir>/../ui-components/src/index',
'^src/(.*)$': '<rootDir>/src/$1',
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we create an empty shared directory in the codebase, move the custom tsconfig.json file in there, and map shared to the appropriate src directory?

We'd essentially require that you explicitly import .oss.tsx files from the "shared" path? Remapping a random+specific name like shared seems a bit less confusing than overriding src? Curious what you think

Copy link
Contributor Author

@salazarm salazarm Aug 5, 2024

Choose a reason for hiding this comment

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

I feel like src makes sense because the folder actually exists. But the concern about using an absolute path in some cases vs relative path in others being confusing is legit... so maybe making the import statement more special is justified... One nice thing about "src/..." is that autocomplete works.

Copy link
Member

@hellendag hellendag left a comment

Choose a reason for hiding this comment

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

Let's move forward with it and iterate. Discussed in a huddle: in followups we can try eliminating the .oss.tsx/.cloud.tsx distinction in favor of a common .shared.tsx, since it's already clear that the oss/cloud files live in their respective repos.

@salazarm salazarm merged commit 825d5f3 into master Aug 6, 2024
2 checks passed
@salazarm salazarm deleted the salazarm/compose-cloud-oss branch August 6, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants