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

Can now export from @primer/react/drafts #1771

Merged
merged 8 commits into from Feb 7, 2022
Merged

Can now export from @primer/react/drafts #1771

merged 8 commits into from Feb 7, 2022

Conversation

pksjce
Copy link
Collaborator

@pksjce pksjce commented Jan 5, 2022

Describe your changes here.

Right now, we need to import draft components from @primer/react/lib-esm/drafts. This is less than ideal.
In our docs, we have aliased @primer/react to the src folder relatively. So it works for our documentation site.
But it clearly does not work elsewhere as demonstrated in #1768

Our module definition right now defines lib-esm/index.js as the main module import in package.json. We need to alias drafts to work as expected.

One option is to use workspaces. I think this is unwieldy for our current usecase.
This blogpost suggested module-alias, but there's a new update to go for subpath-imports.

I'm trying it out with this PR. I need a build package to verify if it's working

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented Jan 5, 2022

⚠️ No Changeset found

Latest commit: 5940a97

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 61.64 KB (0%)
dist/browser.umd.js 62 KB (0%)

@pksjce
Copy link
Collaborator Author

pksjce commented Jan 5, 2022

Update

This does seem to work! Try it for yourself by using this canary version @primer/react@0.0.0-20220572435
https://codesandbox.io/s/objective-dirac-ihz1h?file=/src/App.js

PS - Does this need a changeset?

@pksjce pksjce self-assigned this Jan 5, 2022
@pksjce pksjce added the skip changeset This change does not need a changelog label Jan 5, 2022
@pksjce pksjce marked this pull request as ready for review January 5, 2022 07:42
@pksjce pksjce requested a review from a team January 5, 2022 07:42
Copy link
Contributor

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Yes!! Thank you for this <3

@maximedegreve maximedegreve self-requested a review January 5, 2022 09:59
Copy link
Contributor

@maximedegreve maximedegreve left a comment

Choose a reason for hiding this comment

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

Not 100% sure if it's working or if I'm doing this wrong but...

In my package.json I've added:
"@primer/react": "primer/react#pk/exports",.

But then I get an import error:
Module not found: Can't resolve '@primer/react' in '/workspaces/search/src'

I also can't see a the lib-esm folder in ./node_modules/@primer/react that is defined in exports.

UPDATE: @siddharthkp explained me that we don't support pointing to branches right now and I need to use the generated id however the same error still happens.

@siddharthkp
Copy link
Contributor

@pksjce Should we add IconButton to drafts as well?

@siddharthkp
Copy link
Contributor

siddharthkp commented Jan 5, 2022

Update: Aww, tried to use this with webpack and it doesn't pick it up automatically :( (I think it works on codesandbox because it uses sandpack instead of webpack)

@pksjce
Copy link
Collaborator Author

pksjce commented Jan 6, 2022

@siddharthkp - IconButton is already in there

Also yeah, this exports syntax seems to not be fully supported by webpack 5 and not at all supported in webpack 4. I wonder how the other bundlers will react to this either. Will investigate more.

Until then import {..} from '@primer/react/lib-esm/drafts' is what will work unfortunately.

@maximedegreve
Copy link
Contributor

Sounds good. Before we close this issue can we make sure we update all our docs to have those new instructions on how to import those components?

@siddharthkp
Copy link
Contributor

can we make sure we update all our docs to have those new instructions on how to import those components?

I can create a PR :)

package.json Outdated Show resolved Hide resolved
Try conditional subpaths
Change tsconfig files
Add package.json hack to drafts
@pksjce
Copy link
Collaborator Author

pksjce commented Feb 1, 2022

@siddharthkp , after much trial and error, I seem to have fixed this PR to make it backward compatible with the lib-esm folder imports. Adding

    "./lib-esm/*": {
      "node": ["./lib/*.js","./lib/*/index.js"],
      "default": ["./lib-esm/*.js","./lib-esm/*/index.js"]
    }

seems to have done the trick.
I tested this in memex in this codespace and in a nextjs app too.
I have also updated the documentation for the files in https://github.com/primer/react/pull/1785/files

@siddharthkp siddharthkp self-assigned this Feb 1, 2022
Copy link
Contributor

@siddharthkp siddharthkp 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 gooooo!

@pksjce pksjce enabled auto-merge (squash) February 7, 2022 09:15
@pksjce pksjce removed the request for review from maximedegreve February 7, 2022 09:29
@pksjce pksjce dismissed maximedegreve’s stale review February 7, 2022 09:30

Dismiss sounds so haughty lol. I think I've made the requested changes! Want to merge them!

@pksjce pksjce merged commit 1088ee3 into main Feb 7, 2022
@pksjce pksjce deleted the pk/exports branch February 7, 2022 09:30
wraithgar added a commit to wraithgar/react that referenced this pull request Feb 17, 2022
Invalid typescript version makes `npm ci` fail in the latest npm
npm/cli#4363

Merge conflict was introduced in
primer#1771
siddharthkp pushed a commit that referenced this pull request Feb 17, 2022
Invalid typescript version makes `npm ci` fail in the latest npm
npm/cli#4363

Merge conflict was introduced in
#1771
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
react skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants