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 relative paths in scoped packages #12

Merged
merged 19 commits into from
Feb 1, 2017

Conversation

GeorgeTaveras1231
Copy link
Contributor

@GeorgeTaveras1231 GeorgeTaveras1231 commented Nov 20, 2016

Currently, a path such as "@lennym/npm-sass-test-sass/child-file" errors out. This makes it work(referring to the first commit in this PR), but I have concerns about the growing complexity of the find function.

I'm thinking about abstracting some of the package details such as isScoped, name
, and entrypointLocation (currently known as the location var) into a Package constructor in order to test each of these details in isolation while also simplifying the implementation of find.

What do you think @lennym?


As I mentioned above, I gave a shot at refactoring the code to make it easier to test. I've yet to write unit tests for the logic but while digging into the code I found that a custom file resolution algorithm was being used. After some research, I found that node's own require function could be used to implement this. I like this because this is likely to stay compatible with node/npm future implementations. I tried to describe the changes in this PR 230e069


Finally, I wrote unit tests for all of the new code. It is all runnable via npm run unit-test.

I'm very glad to see that this tool exists, which makes me happy to contribute to it 😄 . @lennym let me know what you think, looking forward to hearing back.

The resolver module implements a path resolving algorithm that
leverages node's own require.

This has a couple benefits:
* more likely to continue working as expected when features are
introduced to npm/node
* reduction of code duplication (npm/node already implements the module
resolution we are interested in for this project)
* allow the use of NODE_PATH variable to control module directory

Additionally, `resolver` leverages `Package` and `Import` constructors
which implement some of the same logic previously found on the `find`
function in `lib/importer.js`. The benefit of creating these
constructors will obvious when unit tests are introduced for this
project.

`Package`: implements logic related to retrieving package data, such as
getting the package.json, and getting the sass 'main' file for a given
package.

`Import`: implements logic to make sense of a sass import.
This allows the tests to work consistently
Also improved style in tests
@GeorgeTaveras1231
Copy link
Contributor Author

Actually, may need to reconsider this change of implementation after considering issues with dependencies of dependencies...

@GeorgeTaveras1231
Copy link
Contributor Author

I have some thoughts in mind for a solution for the problem stated above. Will add tests for nested dependencies in the process since those are missing.

@@ -5,7 +5,8 @@
"main": "index.js",
"bin": "./bin/npm-sass",
"scripts": {
"test": "mocha test/index.js"
"test": "NODE_PATH=$NODE_PATH:\"$(git rev-parse --show-toplevel)/test/fixtures/node_modules\" mocha test/index.js",
"unit-test": "mocha test/units/*.js"
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind refactoring this a little to rename what is now the test task, and then have test run both of the existing test tasks?

That way we can get better coverage in CI.

@lennym
Copy link
Owner

lennym commented Nov 22, 2016

Hi @GeorgeTaveras1231

Thank you so much for your work on this, it's really appreciated. I'm not actually using this module in any of my own projects at the moment (it was written to solve some problems at a former workplace), so it might take a while for me to establish whether it's working as intended.

Do you need a fix for the issue quickly? I'm happy to publish a pre-release version so you can use it straight away. Meanwhile I'll try to review as best I can.


My first quick review is very positive. I like the changes you've made to the general readability of the code. It's certainly a lot easier to follow than my efforts. Will try to look more in depth soon.

@GeorgeTaveras1231
Copy link
Contributor Author

Hey @lennym

Thanks for the response. I'm not in a rush to get this code. Currently, this is something I'm experimenting with, with the hopes that I can pitch it to my organization as a standard tool.

We are currently undergoing a re-architecture of the way we manage front-end assets and so far, the idea of using npm-sass seems really appealing, though I'm not 100% sure as of right now whether we will be using it at a large scale soon.

Don't feel rushed to publish any of it. I will fix the issue I stated above and work on a couple of experiments that do not require this PR to be merged.

If the experiments work out and my team members buy into the idea of using npm-sass regularly, I'd gladly take this project off your hands, especially if you feel burdened by maintaining something you do not use currently. Even if it for a temporary period. How would you feel about that?

@GeorgeTaveras1231
Copy link
Contributor Author

GeorgeTaveras1231 commented Nov 25, 2016

@lennym I believe I am done making code changes to this branch. I updated the test scripts per your advice. Please let me know if there is anything else you would like for me to address.

The major changes since you last saw this PR are:

  • Brought back usage of findup to resolve nested dependencies
  • Introduced Promises for async operations in resolver module (This helped make some of the code clearer)
  • Introduced unit tests for resolving nested dependencies

There are some items that would make sense to address eventually but not as part of this PR.

  • Inconsistent indentation in tests and source files
  • Remove explicit references to'/' path separator from local function.

@tshelburne
Copy link
Contributor

@lennym I just hit this as well - going to test my issue with @GeorgeTaveras1231 's fork and see if it works.

As a side note, @GeorgeTaveras1231, great work - my first time looking at this in a while and it's really clear coding. Love it.

@tshelburne
Copy link
Contributor

tshelburne commented Jan 5, 2017

@GeorgeTaveras1231 @lennym This does appear to work, but I was expecting the relative import to be relative to the "style" property from package.json rather than the module directory itself:

// assuming "style": "dist/scss/library.scss"

// relative to the "style" file
@import '@scope/library/variables/colors';

// relative to the package directory
@import '@scope/library/dist/scss/variables/colors';

My preference would be the former, but it's not a terrible change. That said, I would definitely request documentation for this.

@lennym
Copy link
Owner

lennym commented Jan 6, 2017

Thanks so much for your ongoing work on this @GeorgeTaveras1231 and @tshelburne.

In reference to the options provided above by @tshelburne, I'd actually have a very strong preference for the second one as it works in the same way as require in node when require-ing a non-main module from within a package. My intention for npm-sass has always been for @import to work like require as much as possible.

I should have some time next week to set aside to reviewing this properly, with a view to merging and releasing.

"test": "mocha test/index.js"
"test": "mocha $npm_package_config_unit_test_files $npm_package_config_integration_test_files",
"unit-test": "mocha $npm_package_config_unit_test_files",
"integration-test": "mocha $npm_package_config_integration_test_files"
Copy link
Owner

Choose a reason for hiding this comment

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

TIL. Thanks @GeorgeTaveras1231!

@lennym lennym merged commit fe7c03f into lennym:master Feb 1, 2017
@lennym
Copy link
Owner

lennym commented Feb 1, 2017

Thank you so much for your work on this @GeorgeTaveras1231, I really appreciate it.

I will take some time to address some of your further points above today.

@lennym
Copy link
Owner

lennym commented Feb 1, 2017

I've published your changes with a major version bump to v2.0.0

Thanks again for contributing.

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.

3 participants