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

Official, gradual path to RN packages not publishing untranspiled code #10966

Closed
ljharb opened this issue Nov 16, 2016 · 28 comments
Closed

Official, gradual path to RN packages not publishing untranspiled code #10966

ljharb opened this issue Nov 16, 2016 · 28 comments
Labels
Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.

Comments

@ljharb
Copy link

ljharb commented Nov 16, 2016

The current practice in the RN community for package authors is to publish untranspiled code, to npmignore babelrc, and for the RNP to transpile node_modules.

While this may work great for the RN community, this defies many best practices in the wider node/npm/JS community:

  • package.json's "main" only points to ES5 commonJS
  • npm explore foo && npm install && npm test should work (note this isn't universal, but it's frustrating to be asked to npmignore .babelrc and break this use case due to the RNP)

In addition, this practice kind of permanently cements RN packages with the transpiler settings that the RNP uses - which, since it includes not-yet-stage-4 features, are quite likely to change in a breaking way in the future.

Here's my proposal to enable the benefits of the current approach (cleaner stack traces, optimization opportunities within the RNP, etc) in a way that will allow RN packages to rejoin the wider JS ecosystem:

  1. Designate a new top-level package.json key - let's go with "sources" for the time being. This contains an object. All subkeys of "sources" are optional.
  2. Within "sources":
  3. "raw" points to the entry point for utterly untranspiled code. .babelrc (or other babel config locations), if provided, configure how this code should be transformed.
  4. "react-native-v1" (swap out v1 for any versioning scheme; this frees up the RNP to change their preferred transpiler settings freely.
  5. "node4", "node5", "node6", "node7", etc: code transpiled down to features matching the indicated major node version, following https://www.npmjs.com/package/babel-preset-node7, https://www.npmjs.com/package/babel-preset-node6, etc.
  6. "es2015", "es2016", etc: code transpiled down to features that are included in the given year of publication of the ecmascript standard.
  7. The RNP ships with support for this, with priority: ["react-native-", "node-" (>= the major node version, obv), "main" (with transpiling)]. This change is loudly communicated with blogs, tweets, and documentation.
  8. At some future point (this can be as soon, or as indefinite, as is deemed necessary), the RNP will cease transpiling "main" - since the other "sources" don't require looking for babel configs, this will be when RN packages can (and should) publish their .babelrcs again.

This approach allows for supporting other languages, future transpiler settings, etc, in a nonbreaking way, without infecting the npm ecosystem with untranspiled "main" entry points (inhibiting people's ability to share RN packages across normal react). A package will always be able to choose to omit "main" entirely, which will clearly and explicitly convey to users that their package must not be used without the RNP.

Further reading (credit @ide): https://gist.github.com/ide/e7b9181984933ebb0755c7367a32e7e8

@cpojer
Copy link
Contributor

cpojer commented Nov 16, 2016

I'm fully in support with stopping to transpile node modules. I have to consider a bit more deeply what the actual solution will look like and how many sources on average will be published. We shouldn't encourage people to publish their package with ten sets of differently compiled code.

For RNP, I would recommend the following:

  • At some point in the future, start warning for every node_module that it has to compile. Communicate with the community what we are planning to do and encourage package developers to compile their code. Provide an easy workflow to make this happen.
  • At some point later, we should stop compiling node_modules by default. If something breaks, we'll encourage users to manually whitelist the package that fails.

There is no reason not to enable the escape hatch; especially since there is no point in breaking existing users completely in a way they can't move forward.

@ide
Copy link
Contributor

ide commented Nov 16, 2016

@cpojer I think it would be worse if the packager didn't transpile node_modules at all. We get a lot of benefits because the packager is able to transpile node_modules that are mentioned in the gist. I would be on board with your RNP proposal if this bullet point came before the other two:

  • If a package specifies sources.react-native-vX, transpile the source code from that entry point. Skip the other steps since this package author has explicitly said they want RNP to transpile the code, we don't need to print a warning.

We shouldn't encourage people to publish their package with ten sets of differently compiled code.

Ten might be excessive, for the sake of this conversation wrt React Native why don't we focus on just two? Package authors will choose which sources they want to publish. If someone is authoring react-native-widget they will likely just want to publish main and sources.react-native-v1. A package like lodash might publish just main and es2016. I think the package tarballs will compress quite well too.

@ljharb
Copy link
Author

ljharb commented Nov 16, 2016

I'm not sure i see the harm in publishing packages with multiple sets of differently compiled code, a great many packages already publish two. That said, we don't need to encourage that - it'd be more that we were encouraging them to select one thing, and publish 1) ES5, 2) their choice, and 3) very optionally the "raw". The goal is to enable/encourage publishing ES5, and explicitly informing the RNP of which transpiler settings it should be using.

@skevy
Copy link
Contributor

skevy commented Nov 21, 2016

Somewhat related -- I just made this: https://github.com/skevy/babel-preset-react-native-syntax

This preset would help React Native package authors transpile their code down to what is described here as "sources.react-native-v1".

(Also is useful today, so that people in the RN community who want to use decorators, stage-0 or some stage-1 features in their packages don't have to require their users to use some other babel-preset (like babel-preset-react-native-stage-0), and also can avoid transpiling all the way down to ES5).

@quantuminformation
Copy link
Contributor

Is there a guide for publish RN components to npm?

@hramos
Copy link
Contributor

hramos commented May 25, 2017

Closing this issue because it has been inactive for a while. If you think it should still be opened let us know why.

@hramos hramos closed this as completed May 25, 2017
@ljharb
Copy link
Author

ljharb commented May 25, 2017

@hramos it should absolutely still be open for all the same reasons; RN packages are polluting the ecosystem, and this issue proposes a path to stop that. Please reopen.

@hramos
Copy link
Contributor

hramos commented May 25, 2017

Has there been any movement here? I don't disagree on the usefulness of the work, rather it seems like the issue is not actively being worked on.

@ljharb
Copy link
Author

ljharb commented May 25, 2017

Unfortunately it doesn't seem that way; but issues should be closed when they're resolved or wontfixed, not just because nothing has happened yet.

@hramos
Copy link
Contributor

hramos commented May 25, 2017

Unfortunately we have over 1,000 issues open. Closing inactive issues will allow us to focus on what is actively being worked on. Closing the issue won't delete the conversation, and we can easily re-open when things get moving.

@ljharb
Copy link
Author

ljharb commented May 25, 2017

@cpojer any thoughts on starting this process? The sooner it's begun the more time we have to gradually migrate module authors over.

@skevy
Copy link
Contributor

skevy commented May 25, 2017

@ljharb hey jordan! The packager is actually getting moved out of this repo pretty soon, into it's own thing (which should be easier to iterate on, discuss, take PRs, etc). I'll try to remember to open this issue there (unless you beat me to it). This should be happening in a couple weeks. :)

@ljharb
Copy link
Author

ljharb commented May 25, 2017

Thanks, that sounds great!

@hramos
Copy link
Contributor

hramos commented May 25, 2017

In the process of closing stale issues, I missed that @ljharb was the original author of this issue. I think it's fair to re-open an issue where the OP is actively interested.

I'll re-open this for now, so we don't lose track of it when the packager is moved out.

@hramos hramos reopened this May 25, 2017
lettertwo added a commit to hzdg/react-native-svg-uri that referenced this issue Jun 2, 2017
Because of how babel works, the existence of this config
in node_modules results in conflicts with build tooling
'higher up' in the project dependency tree (namely, it
conflicts with react-native's own build configuration).

Until such time as the RN community abandons the current
practice of publishing untranspiled code to NPM, the best
practice is to exclude any babel config from the published
package to avoid these conflicts.

For more on how babelrc lookup behavior works, see:
https://babeljs.io/docs/usage/babelrc/#lookup-behavior

For more on packaging issues for RN, see:
See facebook/react-native#10966
@hramos
Copy link
Contributor

hramos commented Jul 31, 2017

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

  • Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
  • If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
  • Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.

@ljharb
Copy link
Author

ljharb commented Jul 31, 2017

@hramos any chance this could still be left open? I'm still actively interested.

@hramos hramos added Type: Discussion Long running discussion. and removed Icebox labels Jul 31, 2017
@hramos
Copy link
Contributor

hramos commented Jul 31, 2017

Of course. Re-opening. This issue shouldn't be closed again now that it has the Discussion label. Sorry about that.

@stale
Copy link

stale bot commented Mar 8, 2018

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "For Discussion" or "Good first issue" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 8, 2018
@hawkrives
Copy link
Contributor

stalebot: this issue has the For Discussion label.

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 8, 2018
@quantuminformation
Copy link
Contributor

No supper for stalebot tonight.

@stale
Copy link

stale bot commented Feb 2, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "For Discussion" or "Good first issue" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 2, 2019
@ljharb
Copy link
Author

ljharb commented Feb 2, 2019

This is definitely still forever going to be a problem until the RN team fixes it.

@hramos
Copy link
Contributor

hramos commented Feb 6, 2019

@cpojer what should we do here? This issue seems like a better fit for the discussions-and-proposals repo, which didn't exist at the time the issue was created.

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 6, 2019
@hramos
Copy link
Contributor

hramos commented Feb 6, 2019

(Stale bot is not supposed to nag on Discussion issues, but I recently renamed the "Type: Discussion" label but neglected to update Stale bot's config. Sorry about that)

@cpojer
Copy link
Contributor

cpojer commented Feb 6, 2019

Don't currently have any plans to change what we are doing in RN.

@ljharb
Copy link
Author

ljharb commented Feb 6, 2019

@cpojer can you elaborate on whether and why you've changed your mind since your comment here? Or are you just saying it's not roadmapped right now.

@cpojer
Copy link
Contributor

cpojer commented Feb 7, 2019

I haven't changed my mind, but I doubt anybody is going to change things and I won't have time to spend on this either.

@cpojer
Copy link
Contributor

cpojer commented Mar 19, 2019

I'm gonna close this now as we aren't likely to make any progress on this in the next six months, and this issue has been open for a very long time without any progress. If anybody wants to volunteer to change the status quo, please create an issue with a thorough plan in our discussions-and-proposals so that we can agree and that you can start executing on this.

@cpojer cpojer closed this as completed Mar 19, 2019
@facebook facebook locked as resolved and limited conversation to collaborators Mar 20, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.
Projects
None yet
Development

No branches or pull requests

8 participants