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

Add missing exports for masks and media js files #326

Merged
merged 1 commit into from Dec 19, 2022
Merged

Add missing exports for masks and media js files #326

merged 1 commit into from Dec 19, 2022

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Dec 16, 2022

added media.js, masks.edges.js and masks.corner-cuts.js to exports because they were previously missing and impossible to import.

unsure if this is the right convention (i couldn't find any previous examples with multiple dots in the file name)

also, i didn't include it in this pr but it might help to add a catch-all to the export map. this will make sure everything is exposed (unless some things are purposely hidden).

"./": "./"

@stackblitz
Copy link

stackblitz bot commented Dec 16, 2022

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Owner

@argyleink argyleink 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 catching and adding those 🙂
this looks good to me except the duplicate media entry.

Side thought:
maybe i should make a separate issue to normalize the naming convention so all the js modules use the esm/ prefix 🤔

Side Q:
what env are you in that you can't reach into the package and grab any file you want? which env is forcing you only to importing exported items? import "open-props/docsite/syntax-highlight.css"; should just work if someone wants it to?

package.json Outdated
@@ -39,6 +39,9 @@
"./src/sizes": "./src/props.sizes.js",
"./src/svg": "./src/props.svg.js",
"./src/zindex": "./src/props.zindex.js",
"./src/media": "./src/props.media.js",
Copy link
Owner

Choose a reason for hiding this comment

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

this was already exposed as "./esm/media": "./src/props.media.js", 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh, i missed that. undone! 👍

if you would like everything to be under esm/, i can update the other two as well.

the documentation mentions src/ fwiw

Copy link
Owner

Choose a reason for hiding this comment

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

i'll fix in #327, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll fix in #327, thanks!

that is technically a breaking change, jsyk (after you mentioned esm/media, i added it in my script. now if i upgrade to 1.5.2, it will break my script)

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, it is technically one! but i also silently released it, never documented it and have discord if anyone else finds the feature and change.

@mayank99
Copy link
Contributor Author

Side thought: maybe i should make a separate issue to normalize the naming convention so all the js modules use the esm/ prefix 🤔

makes sense to me! but the old export paths would need to be kept around to avoid breaking changes.

Side Q: what env are you in that you can't reach into the package and grab any file you want? which env is forcing you only to importing exported items? import "open-props/docsite/syntax-highlight.css"; should just work if someone wants it to?

it's just a plain node esm environment (i'm building a wrapper over open-props using an .mjs script).

import MaskEdges from 'open-props/src/props.masks.edges.js';
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]:
  Package subpath './src/props.masks.edges.js' is not defined by "exports" in open-props/package.json

@argyleink
Copy link
Owner

which bundler is doing that npm lookup?

also, adding "./": "./" sounds good to me. i thought folks generally could already snag what they needed, but sounds like your bundler is restricted to whats in the exports, or limited, either way it's a bummer for you when the files are right there on disk..

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

can include "./": "./" in this pr or another, no matter to me, but this one is g2g 👍🏻

added `media.js`, `masks.edges.js` and `masks.corner-cuts.js` to exports because they were previously missing and impossible to import.
@mayank99
Copy link
Contributor Author

mayank99 commented Dec 19, 2022

which bundler is doing that npm lookup?

there is no bundler. it is just a plain node script running in ESM mode. the command i run is node generateScss.mjs.

if there was a bundler though, i do know vite/astro use plain node ESM for SSR

can include "./": "./" in this pr or another, no matter to me, but this one is g2g 👍🏻

done!

@argyleink argyleink merged commit 8daf651 into argyleink:main Dec 19, 2022
@mayank99 mayank99 deleted the patch-1 branch December 19, 2022 20:14
@argyleink
Copy link
Owner

released in v1.5.3 👍🏻

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.

None yet

2 participants