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

Fix for #322 #323

Merged

Conversation

matthewdavidson
Copy link
Contributor

@matthewdavidson matthewdavidson commented Nov 22, 2016

This PR is by no means complete - I've spent a bit of time familiarising myself with the codebase and this is my quick hack to get things working. However as a result I have some observations...

Whilst I guess it's relatively uncommon, lodash is a good example of a package that exposes entry points multiple levels deep such as lodash/fp/object or lodash/fp/curryN. The method currently used to resolve packages - by reading the components package.json - wont be enough to future proof this use case as it's not feasible to statically analyse the package for all of the possible entry points (at n levels of nesting).

An alternative approach would be to refactor the require-wrapper to pass any paths through to require as long as they match dependencies that are listed in the components package.json as well as conforming to the registries whitelist of dependencies.

@matteofigus thoughts?

@matthewdavidson matthewdavidson changed the title Partial fix for #322 [wip] Partial fix for #322 Nov 22, 2016
@matteofigus
Copy link
Member

Hi @matthewdavidson thanks for the PR. I agree about your proposal.

Summarising, I would be happy if both component and registry maintainer declare "lodash" as dependency, and then server.js is able to require anything that matches "lodash" or "lodash/anything" or "lodash/anything/anything(etc)...".

@matthewdavidson matthewdavidson changed the title [wip] Partial fix for #322 Fix for #322 Dec 20, 2016
@matthewdavidson
Copy link
Contributor Author

Apologies for the delay - I've updated the PR.

dependency-resolver.js has been ditched in favor of require-wrapper.js serving only to validate any requires against the dependency whitelist.

Copy link
Member

@matteofigus matteofigus left a comment

Choose a reason for hiding this comment

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

I think all looks good excellent!
I still would like 2 little changes:

  1. A little cleanup on the npm-shrinkwrap (see inline comment)
  2. I see there are quite substantial changes on the dependencies management and lack of acceptance tests on that side. To improve that, I created another PR (Acceptance extra test #334) - so I would like that to be merged first so that rebasing this would run that test too.

"version": "2.0.1",
"from": "require-package-name@latest",
"resolved": "https://registry.npmjs.org/require-package-name/-/require-package-name-2.0.1.tgz"
},
Copy link
Member

Choose a reason for hiding this comment

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

Hi, many things have changed here but for the sake of this PR I would prefer to have only this five lines added here.

To summarise, usage of npm-shrinkwrap is not ideal and the only reason for having it at the moment is to lock a couple of sub-dependencies to versions that are safe (so that security checks are all green) - until we pay technical dept and upgrade them on separate PRs (jade to pug, express@4) - so that security checks can go green without overrides again (and we can get rid of shrinkrap).

So, for this PR, can I ask you to undo your changes to this file (probably done by automatically re-generating the shrinkrap on top of the package) and just manually add the require-package-name stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (i think).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, can you just pull in the latest master so we can get that extra test ;)

@matteofigus matteofigus merged commit 8f31d68 into opencomponents:master Dec 21, 2016
@matteofigus
Copy link
Member

Thanks for taking care of this @matthewdavidson - just published this to a new version

@matthewdavidson
Copy link
Contributor Author

👍 🎉

@matthewdavidson matthewdavidson deleted the require-nested-paths-fix branch December 21, 2016 22:16
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