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

Allow importing package.json #2468

Merged
merged 15 commits into from Aug 2, 2017

Conversation

Projects
None yet
7 participants
@iamdoron
Copy link
Contributor

commented Jun 4, 2017

Hi,

Basically, allow to import package.json.
For example, from src/App.js:

import { version } from '../package.json'

Only the app own package.json is allowed to be imported.

It closes #2466

import_packagejson

@iamdoron iamdoron referenced this pull request Jun 4, 2017

Closed

App Version #2466

@rogovdm

This comment has been minimized.

Copy link

commented Jun 4, 2017

It it a safe practice to do so?

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2017

@rogovdm, I guess it depends what kind of date is in the package.json. Also, in terms of bundle size it might not be efficient to import the whole package.json.

Maybe it's better to only include the version property from the package.json

@Timer Timer added this to the 1.0.x milestone Jun 19, 2017

@gaearon
Copy link
Member

left a comment

I don't think this is correct. descriptionFileRoot is package.json even if I import ../lol.json. And it lets me do that. So this is probably the wrong thing to look at.

I assume that the correct thing to look at would be requestRelative calculated below, and if it's equal exactly to ../package or ../package.json (since we can't use Webpack resolving mechanism yet).

@iamdoron iamdoron force-pushed the iamdoron:master branch from f58f9d0 to 9ef9fcb Jun 22, 2017

const requestRelativeToRoot = path.relative(
request.descriptionFileRoot,
requestFullPath
);

This comment has been minimized.

Copy link
@iamdoron

iamdoron Jun 22, 2017

Author Contributor

@gaearon you are totally right. My previous code wasn't working . I'm not sure what I did there, but this is what I meant to do - to check this is the package.json that's in the root. I didn't want to assume package.json always lives in ../src in case the src is more than one level deep like true/src

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

I don't understand newly pushed commits. I was referring to using existing variable (which has the right thing). You renamed an old variable but it still calculates the wrong thing.

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2017

I think it's OK, but maybe I'm missing something
there are two types or "relatives".
relative to src
relative to root

I wanted to check this is the package.json that resides in root:

const requestRelativeToRoot = path.relative(
        request.descriptionFileRoot,
        requestFullPath
);
@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

I'm not sure myself. Just make sure importing ../somethingelse.json is still an error.

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2017

now when I run it:

require("../../package_.json")

Module not found: You attempted to import ../../package_.json

require("../package_.json")

Module not found: You attempted to import ../package.json

and require("../../package.json") works because it's inside the root

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

Wouldn't it be easier to simply use this variable and add:

      if (requestRelative === `..${path.sep}package.json`) {
        return callback();
      }

I might be missing something obvious. I have not tested this.

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2017

@Timer I wanted it to check from the root and not src/../package.json in case the source folder is two levels or more from the root
but maybe it's enough to assume that src/../package.json is the right one

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

Good point, @iamdoron.
Maybe a regex pattern like this?

      if (/^(..[/|\\])+package.json$/.test(requestRelative)) {
        return callback();
      }

@iamdoron iamdoron force-pushed the iamdoron:master branch 2 times, most recently from 9be52d8 to be60e23 Jun 22, 2017

);
const requestRelative = path.relative(appSrc, requestFullPath);
if (/^(..[/|\\])+package(.json)?$/.test(requestRelative)) {

This comment has been minimized.

Copy link
@iamdoron

iamdoron Jun 22, 2017

Author Contributor

@Timer I changed it to regex, I added '?' in (.json)? to make it optional. that way you can require("../package")

This comment has been minimized.

Copy link
@Timer

Timer Jun 22, 2017

Collaborator

It should always include the extension no matter what.

@viankakrisna

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2017

How about adding excludes option to ModuleScopePlugin?

      new ModuleScopePlugin({
        appSrc: paths.appSrc,
        excludes: [paths.appPackageJson]
      }),
      if (excludes.includes(request.path)){
        return callback()
      }

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

@viankakrisna that's probably best long-term; what do you think @iamdoron?

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

What's the status on this? What are requested changes?

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2017

We should strictly enforce the .json extension so people don't use this as an escape hatch to import package.js from the root.

@Timer Timer removed this from the 1.0.x milestone Jun 29, 2017

@@ -13,8 +13,9 @@ const chalk = require('chalk');
const path = require('path');

class ModuleScopePlugin {
constructor(appSrc) {
constructor(appSrc, allowedPaths = []) {

This comment has been minimized.

Copy link
@Timer

Timer Jun 29, 2017

Collaborator

Small nit, how about allowedFiles? Paths makes it sound like you can give it a directory.

@@ -57,7 +57,7 @@ module.exports = {
```


#### `new ModuleScopePlugin(appSrc: string)`
#### `new ModuleScopePlugin(appSrc: string, allowedPaths: [string])`

This comment has been minimized.

Copy link
@Timer

Timer Jun 29, 2017

Collaborator

Ditto, allowedFiles. And we should probably let them know it's optional allowedFiles?: string[].

@br0p0p

This comment has been minimized.

Copy link

commented Jul 31, 2017

What's the status on this?

Timer added some commits Aug 2, 2017

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2017

@br0p0p waiting for CI to pass, now. 😄 thanks for the ping!

@Timer Timer merged commit 24b18ae into facebook:master Aug 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Timer

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2017

Thanks for this, @iamdoron!

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2017

Hi there! This change is out in react-scripts@1.0.11; please give it a go! Thanks.

JohnNilsson referenced this pull request in JohnNilsson/create-react-app-typescript Aug 9, 2017

Allow importing package.json (#2468)
* Allow importing package.json

* Remove package.json import from App.js template

* fix importing package.json

* Rename variable to reflect path is relative to root

* Check for both package & package.json in ModuleScopePlugin

* Use regex to check relative path to package.json

* Strictly enforce package.json extension on scope plugin

* Add allowedPaths to ModuleScopePlugin ctor and use it to allow app package.json

* Remove package.json import from App.js template

* Add package.json to react-scripts/template, show package version and name in the template

* Remove import package.json from template

* Remove template/package.json and its references in code

* Update ModuleScopePlugin.js

* Update README.md

someden pushed a commit to someden/create-react-app that referenced this pull request Aug 9, 2017

Allow importing package.json (facebook#2468)
* Allow importing package.json

* Remove package.json import from App.js template

* fix importing package.json

* Rename variable to reflect path is relative to root

* Check for both package & package.json in ModuleScopePlugin

* Use regex to check relative path to package.json

* Strictly enforce package.json extension on scope plugin

* Add allowedPaths to ModuleScopePlugin ctor and use it to allow app package.json

* Remove package.json import from App.js template

* Add package.json to react-scripts/template, show package version and name in the template

* Remove import package.json from template

* Remove template/package.json and its references in code

* Update ModuleScopePlugin.js

* Update README.md
@br0p0p

This comment has been minimized.

Copy link

commented Aug 9, 2017

@Timer @iamdoron great, thanks for this!

zangrafx added a commit to absolvent/create-react-app that referenced this pull request Aug 13, 2017

Merge commit 'bfaee410c502a95076a6bd89721c76ca08e15f7b'
* commit 'bfaee410c502a95076a6bd89721c76ca08e15f7b': (39 commits)
  Publish
  Prepare for 1.0.11 release (facebook#2924)
  Update dev deps (facebook#2923)
  Update README.md
  Use env variable to disable source maps (facebook#2818)
  Make formatWebpackMessages return all messages (facebook#2834)
  Adjust the `checkIfOnline` check if in a corporate proxy environment (facebook#2884)
  Fix the order of arguments in spawned child proc (facebook#2913)
  Feature/webpack 3 4 (facebook#2875)
  Allow importing package.json (facebook#2468)
  Re-enable flowtype warning (facebook#2718)
  Format UglifyJs error (facebook#2650)
  Unstage yarn.lock pre-commit (facebook#2700)
  Update README.md
  Update README.md
  Add Electrode to alternatives (facebook#2728)
  Fix parsing HTML/JSX tags to real elements (facebook#2796)
  Update webpack version note (facebook#2798)
  Use modern syntax feature (facebook#2873)
  Allow use of scoped packages with a pinned version (facebook#2853)
  ...

# Conflicts:
#	packages/react-scripts/config/webpack.config.dev.js
#	packages/react-scripts/config/webpack.config.prod.js
#	packages/react-scripts/package.json

JohnNilsson referenced this pull request in JohnNilsson/create-react-app-typescript Sep 9, 2017

Allow importing package.json (#2468)
* Allow importing package.json

* Remove package.json import from App.js template

* fix importing package.json

* Rename variable to reflect path is relative to root

* Check for both package & package.json in ModuleScopePlugin

* Use regex to check relative path to package.json

* Strictly enforce package.json extension on scope plugin

* Add allowedPaths to ModuleScopePlugin ctor and use it to allow app package.json

* Remove package.json import from App.js template

* Add package.json to react-scripts/template, show package version and name in the template

* Remove import package.json from template

* Remove template/package.json and its references in code

* Update ModuleScopePlugin.js

* Update README.md

Ayc0 pushed a commit to Ayc0/create-react-app that referenced this pull request Sep 18, 2017

Allow importing package.json (facebook#2468)
* Allow importing package.json

* Remove package.json import from App.js template

* fix importing package.json

* Rename variable to reflect path is relative to root

* Check for both package & package.json in ModuleScopePlugin

* Use regex to check relative path to package.json

* Strictly enforce package.json extension on scope plugin

* Add allowedPaths to ModuleScopePlugin ctor and use it to allow app package.json

* Remove package.json import from App.js template

* Add package.json to react-scripts/template, show package version and name in the template

* Remove import package.json from template

* Remove template/package.json and its references in code

* Update ModuleScopePlugin.js

* Update README.md

kasperpeulen added a commit to kasperpeulen/create-react-app that referenced this pull request Sep 24, 2017

Allow importing package.json (facebook#2468)
* Allow importing package.json

* Remove package.json import from App.js template

* fix importing package.json

* Rename variable to reflect path is relative to root

* Check for both package & package.json in ModuleScopePlugin

* Use regex to check relative path to package.json

* Strictly enforce package.json extension on scope plugin

* Add allowedPaths to ModuleScopePlugin ctor and use it to allow app package.json

* Remove package.json import from App.js template

* Add package.json to react-scripts/template, show package version and name in the template

* Remove import package.json from template

* Remove template/package.json and its references in code

* Update ModuleScopePlugin.js

* Update README.md

swengorschewski referenced this pull request in swengorschewski/cra-typescript-electron Oct 16, 2017

Allow importing package.json (#2468)
* Allow importing package.json

* Remove package.json import from App.js template

* fix importing package.json

* Rename variable to reflect path is relative to root

* Check for both package & package.json in ModuleScopePlugin

* Use regex to check relative path to package.json

* Strictly enforce package.json extension on scope plugin

* Add allowedPaths to ModuleScopePlugin ctor and use it to allow app package.json

* Remove package.json import from App.js template

* Add package.json to react-scripts/template, show package version and name in the template

* Remove import package.json from template

* Remove template/package.json and its references in code

* Update ModuleScopePlugin.js

* Update README.md

jdcrensh added a commit to jdcrensh/create-react-app that referenced this pull request Oct 24, 2017

Allow importing package.json (facebook#2468)
* Allow importing package.json

* Remove package.json import from App.js template

* fix importing package.json

* Rename variable to reflect path is relative to root

* Check for both package & package.json in ModuleScopePlugin

* Use regex to check relative path to package.json

* Strictly enforce package.json extension on scope plugin

* Add allowedPaths to ModuleScopePlugin ctor and use it to allow app package.json

* Remove package.json import from App.js template

* Add package.json to react-scripts/template, show package version and name in the template

* Remove import package.json from template

* Remove template/package.json and its references in code

* Update ModuleScopePlugin.js

* Update README.md

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.