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

CommonJS Support #93

Merged
merged 7 commits into from
Nov 29, 2021
Merged

Conversation

finkef
Copy link
Contributor

@finkef finkef commented Nov 18, 2021

This PR builds on #91 and removes the react-native dependency from the create function. The create function now also requires passing in the device platform as second parameter.
To stay backwards-compatible, we construct and export a new create function that injects the current platform and matches the previous function signature in index.ts, keeping the original function unaware of react-native.
With these changes, we can now import a node-compatible version using import create from "twrnc/create".

Closes #92.

@jaredh159
Copy link
Owner

Code looks pretty good. You can fix the CI failure by running npm run format and npm run lint:fix.

Can you think of a way to write a test that would prevent a regression? I'm wondering if a few months or years down the road me, or some contributor shuffled files around, or pulled in react-native somewhere, it would not be obvious that we would be breaking compatibility with node. Is there any way to capture this in a test that would fail in that case?

@jaredh159
Copy link
Owner

also, i checked out your PR branch and released another beta version. you can install and test with npm i twrnc@next.

@finkef
Copy link
Contributor Author

finkef commented Nov 18, 2021

Perfect, testing twrnc@next in a bit.
Not sure about a test since jest will easily resolve all modules with ts-jest. We could spawn a node process requiring the compiled version of create which should immediately throw if any ES module is referenced. I'll run some tests locally and add a simple test. The downside is however that we need the compiled output when running the test, so we'll probably need to run the compilation from the test suite itself, maybe in a separate folder and cleanup after.

@finkef
Copy link
Contributor Author

finkef commented Nov 18, 2021

Works! Now, it creates a temporary directory under .twrnc in the project root, compiles only the CommonJS and tries to require() it from a node one-liner. If any ES module is imported, it fails properly.

@jaredh159
Copy link
Owner

thanks, this looks really good. i'm hoping to check it out locally and do some testing with it this afternoon, and then hopefully merge.

is the babel plugin you're working on an open source project? or is it embedded in some other app? I'd be interested in looking at it if it was open source.

@finkef
Copy link
Contributor Author

finkef commented Nov 19, 2021

Yes absolutely! Want to polish some things and add docs before publishing however, so it's not public yet. Any chance you could publish a new release with the most recent commit I added to this branch? Though it seemed it was working out of the box on all platforms, it still produced some errors without the exports in the package.json.

@jaredh159
Copy link
Owner

Any chance you could publish a new release with the most recent commit I added to this branch?

Done, 2.1.0-beta.5 is out, install with npm i twrnc@next.

@jaredh159
Copy link
Owner

sent your PR a PR with a proposed simplification of the node compat test, let me know your thoughts when you have a sec:

finkef#1

move test for node commonjs compat into gh action ci
@finkef
Copy link
Contributor Author

finkef commented Nov 20, 2021

Yeah, that's much better than having it in a jest test, merged into this PR!

@finkef
Copy link
Contributor Author

finkef commented Nov 20, 2021

FYI, just published the babel macro here: react-native-tailwind.macro 🎉
Would love to hear your thoughts!

@jaredh159
Copy link
Owner

hey, i got this error/warning when testing the latest set of changes by installing a new beta build into a test RN app:

warn Package twrnc has been ignored because it contains invalid configuration. Reason: Package subpath './package.json' is not defined by "exports" in /Users/jared/x-pkg/tw/app/node_modules/twrnc/package.json

I'm not super familiar with the stuff you did with the package.json exports field. Do you know why I'd be getting this error?

@jaredh159
Copy link
Owner

looks like it might be an issue with react-native, see: react-native-community/cli#1168 and uuidjs/uuid#444 for some more context. non-web RN is the main target of this library, so I definitely want that path to be the happy path and work without any funkiness or warnings.

it might be enough to just supply a path, like here: tobua/epic-mobx@4645f51

@jaredh159
Copy link
Owner

i tried adding "./package.json": "./package.json" to the "exports" field, and the metro warning went away, but trying to run the sample app failed. Are you not getting these failures? I'm guessing since your library is just consuming the new create file, you're not seeing it. But I'm getting this using the library directly in RN:

Screen Shot 2021-11-22 at 10 51 28 AM

@finkef
Copy link
Contributor Author

finkef commented Nov 22, 2021

I'm running 2.1.0-beta.5 in an Expo project without any issues. 🤔
It might be that we need to also explicitly add "./tailwind.config.js": "./tailwind.config.js" to exports.

@finkef
Copy link
Contributor Author

finkef commented Nov 22, 2021

Just realized that it's complaining about the create import. Did you try cleaning the metro/babel cache by starting the bundler with --reset-cache? No idea if that could be an issue, but since we're messing with exports 🤷

@jaredh159
Copy link
Owner

good thought on the --reset-cache. but, i tried it, and no dice. my test app is not an Expo app, it's vanilla RN, fwiw.

@finkef
Copy link
Contributor Author

finkef commented Nov 22, 2021

Just tried against a fresh react-native init (RN 0.66.3) and also no issues with that version. 🤔

Here's what I'm doing in my App.js:

import React from 'react';
import { View } from 'react-native';

import tw from 'twrnc';

const App = () => <View style={tw`flex-1 bg-blue-500`} />;

export default App;

@jaredh159
Copy link
Owner

can you try creating a customized version of the tw function by doing something like:

const tw = create(require(`../tailwind.config`));

@finkef
Copy link
Contributor Author

finkef commented Nov 22, 2021

No problem either. I'm running plain JS however, for some reason I keep getting errors bootstrapping a TS project, so can't confirm with TS yet.

@finkef
Copy link
Contributor Author

finkef commented Nov 22, 2021

Finally got a new TS project up and running and also no issues...

import React from 'react';
import {View} from 'react-native';

import {create, useDeviceContext} from 'twrnc';

const tw = create(require('./tailwind.config'));

const App = () => {
  useDeviceContext(tw);

  return <View style={tw`flex-1 bg-blue-500`} />;
};

export default App;

@finkef
Copy link
Contributor Author

finkef commented Nov 22, 2021

Do you have the package installed via npm or linked locally, btw? Have had some troubles with that in the past.

@jaredh159
Copy link
Owner

Ack, shoot, I figured it out. I had a random dir lying around in my app root directory from some debugging I had been doing, and for some reason metro was getting super confused and poking around in there. When I removed that, it started working. Sorry to waste your time.

I still think the package.json mapping might be necessary, I'll add that.

However, I'm going to be unavailable the rest of this week, so I think i'm going to hold off publishing a non-beta version until next week. Hopefully you're OK depending on the beta release until then.

Your babel macro library looks really slick! Nice work. 👍

@finkef
Copy link
Contributor Author

finkef commented Nov 24, 2021

Yes, good call on the package.json. Sounds good to me!

@jaredh159 jaredh159 merged commit 999a6eb into jaredh159:master Nov 29, 2021
@jaredh159
Copy link
Owner

merged. thanks for your work on this. i'll get a release out shortly. 👍

@jaredh159
Copy link
Owner

2.1.0 released.

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.

2 participants