-
Notifications
You must be signed in to change notification settings - Fork 2
add export field to package.json #496
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
Conversation
Coverage Report
File CoverageNo changed files found. |
ascibisz
left a comment
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 looks good to me! thanks for the quick turnaround on the fix
package.json
Outdated
| }, | ||
| "exports": { | ||
| ".": "./es/index.js", | ||
| "./style/style.css": "./style/style.css" |
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 odd to me because of the "files" section above, that has both "es" and "style" in it.. it looks like this is pushing es/ up one level relative to style/
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.
Oh you're right, so this branch is currently putting it at the top level and in /es. The /es one has always been there but webpack wasn't using it. It works...
But I found another way to fix it which is to just import import "./style/style.css"; in src/index.ts...
Feels too simple, so I'm going to keep rebuilding and repacking and re installing until I'm sure but it seems like the better way to do this.
|
Importing the stylesheet into index.ts: locally the builds, installs and tests are working, but CI tests fail to resolve import. It's notable to me that Vite building the test bed resolves the import just fine where webpack for the website doesn't. I'm down to look at the simularium website moving to Vite, or otherwise tweaking that build, but I won't be able to get that done today. Current patch option that makes simularium website build happy is the "exports" field with styles at both levels. or leave this open for a minute |
|
Here is my best-thought/understanding about this issue and how to resolve it. Issue 1: When we switched to ES modules, #431, we broke imports that were not in Issue 2: We thought we fixed that by putting stylesheet in My proposed fix in commit ab61007:
allows this import to resolve: import Pros: There is no duplication of stylesheet, uses exports field which should have been part of our ES migration, installing local build folder resolves the same way published packages do. Final proof will be to release a patch and install it in the website to make sure that CI/CD pipeline is also happy. |
| }, | ||
| "exports": { | ||
| ".": "./es/index.js", | ||
| "./style/style.css": "./es/style/style.css" |
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.
Seeing this makes me wonder if you could have the same effect if you just had one single export alias from "." to "./es"
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.
Possible, and I could test that if you like, but I think that would allow importing from everything in /es rather than just the stuff we choose to expose in index.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.
okay, then in that case I think I like your fix as is, assuming it works in all the cases we care about! Or if @meganrm has any better ideas
toloudis
left a comment
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 an elegant thing that doesn't really require special export aliasing config, but I'm happy this makes things work
Time Estimate or Size
small
Problem
Installing current viewer version is breaking the website build
It seems the way we are exporting our stylesheet from the viewer, the website bundler can't grab it correctly.
https://webpack.js.org/guides/package-exports/
I was able to inspect
/esin the node_modules of the website and see the file we were trying to import, but the bundled website was not including it without the explicit export in package.json of the viewer.Solution
Adding "exports" field to package.json tells webpack to bundle the stylesheet during website build.
Locally I can test the fix both a dev build via a directory and a
.tgz, but I get a little dizzy making changes to both repos, rebuilding, repacking, and reinstalling so I would appreciate a double check, we can also try by just patching the published version.The unfortunate thing, which I also don't understand, is that this seemed to trigger some breakages in the typing on our styled components in the website.
I have no idea why, it does make sense because we had a mismatch in our v6
styled-componentsand@types/styled-componentsversion, but I don't know why the viewer build seemed to trigger it.PR to fix website:
simularium/simularium-website#649
When we publish the viewer patch, I will install it on that branch, and then hopefully we will see a successful website build.
Final confirmation will be a successful website build with the ability to alt-ctrl-1 to see the advanced controls.