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
Merged

Allow importing package.json #2468

merged 15 commits into from
Aug 2, 2017

Conversation

iamdoron
Copy link
Contributor

@iamdoron iamdoron 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

@RohovDmytro
Copy link

It it a safe practice to do so?

@iamdoron
Copy link
Contributor Author

iamdoron 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
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

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).

const requestRelativeToRoot = path.relative(
request.descriptionFileRoot,
requestFullPath
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

gaearon 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
Copy link
Contributor Author

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
Copy link
Contributor

gaearon commented Jun 22, 2017

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

@iamdoron
Copy link
Contributor Author

iamdoron 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
Copy link
Contributor

Timer 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
Copy link
Contributor Author

@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
Copy link
Contributor

Timer 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 master branch 2 times, most recently from 9be52d8 to be60e23 Compare June 22, 2017 20:56
);
const requestRelative = path.relative(appSrc, requestFullPath);
if (/^(..[/|\\])+package(.json)?$/.test(requestRelative)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It should always include the extension no matter what.

@viankakrisna
Copy link
Contributor

How about adding excludes option to ModuleScopePlugin?

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

@Timer
Copy link
Contributor

Timer commented Jun 22, 2017

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

@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

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

@Timer
Copy link
Contributor

Timer 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.

Copy link
Contributor

@Timer Timer left a comment

Choose a reason for hiding this comment

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

Looks great!

@Timer Timer modified the milestones: 1.0.10, 1.0.x Jun 29, 2017
@@ -13,8 +13,9 @@ const chalk = require('chalk');
const path = require('path');

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

Choose a reason for hiding this comment

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

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])`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@br0p0p
Copy link

br0p0p commented Jul 31, 2017

What's the status on this?

@Timer
Copy link
Contributor

Timer 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
@Timer
Copy link
Contributor

Timer commented Aug 2, 2017

Thanks for this, @iamdoron!

@Timer
Copy link
Contributor

Timer commented Aug 9, 2017

Hi there! This change is out in [email protected]; please give it a go! Thanks.

JohnNilsson referenced this pull request in JohnNilsson/create-react-app-typescript Aug 9, 2017
* 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
Copy link

br0p0p 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
* 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

* 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 pushed a commit to kasperpeulen/create-react-app that referenced this pull request Sep 24, 2017
* 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

* 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App Version
7 participants