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

Leave out .js so Metro can resolve native variant #7675

Merged
merged 3 commits into from Jul 3, 2018

Conversation

Projects
None yet
2 participants
@hypest
Contributor

hypest commented Jul 2, 2018

Description

This PR drops the .js extension from @wordpress/element's package.json entrypoint definitions so when used in the mobile RN app the correct module (index.native.js) can be resolved by Metro.

The same treatment should probably be performed in all packages, or at least the ones that have native mobile specializations but, let's do that step by step starting with the element package that currently is breaking the RN app (at the time of writing, commit hash).

How has this been tested?

Ran npm run dev successfully.

Types of changes

Bug fix: Modify the @wordpress/element package entrypoint definitions (main, module).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@hypest hypest requested a review from gziolo Jul 2, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 3, 2018

We can also try using react-native field instead as suggested here: facebook/metro#59 (comment).

For some reasons they usually add an extension in the path provided in the main field. I’m not sure why. We probably should to contact npm support to confirm it.

@hypest

This comment has been minimized.

Contributor

hypest commented Jul 3, 2018

Thanks for the research @gziolo ! I hated to have the .js removed but that was the only solution I came up with after failing to find a canonical one :(

I will test out the react-native field option and will revise this PR if successful.

hypest added some commits Jul 3, 2018

Use the `react-native` entrypoint for RN
And let it point to the extension-less index module so Metro can decide
which module to resolve to, as normal.
@hypest

This comment has been minimized.

Contributor

hypest commented Jul 3, 2018

Reverted the previous change and implemented the solution based on the react-native package.json entry point @gziolo , ready for a review!

I opted for a "extension-less" entrypoint for RN to allow Metro use its resolve policy as normal. Let me know if you have other thoughts on this.

I tested the solution against this PR over at the RN app repo.

@gziolo

Technically, you can use build/index.native.js as we don't have other overrides. It's colocated with the code, so you know upfront what options are there. If you prefer it, we can leave it as is 👍

Do we need to add react-native fields to other packages that use RN overrides? I'd prefer to include them all in this PR.

@gziolo gziolo added this to the 3.2 milestone Jul 3, 2018

@gziolo

gziolo approved these changes Jul 3, 2018

@hypest

This comment has been minimized.

Contributor

hypest commented Jul 3, 2018

Technically, you can use build/index.native.js as we don't have other overrides. It's colocated with the code, so you know upfront what options are there. If you prefer it, we can leave it as is 👍

True, we can also point to the full filename. I'm not yet 100% which approach is best for the RN case; we should keep an open eye on this. Let's leave it extension-free for now since it feels more Metro-native (but not as much npm-native) and we can revise in the future if new info comes up.

Do we need to add react-native fields to other packages that use RN overrides? I'd prefer to include them all in this PR.

Currently, there is no other package in the packages folder that has native mobile files. That said, we can still add the entrypoint anyway (provided we use the extension-free form) and will work just fine since Metro will pick up the web variant as normal. Let me know if you want to go that route.

Edit: over DM, @gziolo gave a 👍 to merge as is so, will do that now.

@hypest hypest self-assigned this Jul 3, 2018

@hypest hypest merged commit fdf752c into master Jul 3, 2018

2 checks passed

codecov/project 46.78% (-0.33%) compared to 060fb5f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hypest hypest deleted the rnmobile/element-rn-compat-entrypoints branch Jul 3, 2018

@hypest

This comment has been minimized.

Contributor

hypest commented Jul 3, 2018

@gziolo , I just found out that the react-native field/entrypoint is not what's picked up by the jest tests of the RN app repo. Also tried the browser entrypoint but doesn't work either. One needs to leave the main entrypoint extension-less for the jest tests to succeed. So, we need some more thinking to do on this one.

An idea is to try is to leave index.js as the entry point for all but move its contents to a module imported by index.js. That new module can have the .native.js variant and Metro can pick it up normally.

Edit: here's the error occurring in Travis for the RN app: https://travis-ci.org/wordpress-mobile/gutenberg-mobile/jobs/399907186#L523

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 4, 2018

I see, I wrote to one of the maintainers of Jest, let's see if they have any solution ready. I couldn't find anything like that in their repository.

@hypest

This comment has been minimized.

Contributor

hypest commented Jul 6, 2018

I just found out that the react-native field/entrypoint is not what's picked up by the jest tests of the RN app repo.

Another false negative @gziolo 😢 . I've updated the RN app (see wordpress-mobile/gutenberg-mobile#61 (comment)) to consume packages directly from /packages and to point to the latest from #7691 and both the RN app and the testsuite are green.

I think we can probably lower the priority of fixing this. We'll still want to remove the highly undocumented react-native entry point and use a more robust solution based on the main entrypoint, but we can leave it aside for a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment