Export solid-permissions constants by dmitrizagidulin · Pull Request #129 · solid-contrib/solid-client

dmitrizagidulin · 2016-11-01T18:47:03Z

Export as solid.acl.* various constants, Authorization, PermissionSet, access modes
Add tests for above
Add babel-cli, point tests to lib/ instead of src/, update configs
Remove unused test/resources/acl-container-ttl.js file (was moved to solid-permissions previously)

Closes issue #125.

dmitrizagidulin · 2016-11-01T18:53:32Z

Tagging @dan-f / @nicola / @deiu for review.

dan-f

I'm stoked about adding a lib directory for transpiled JS, 👍 to that!

That said I'm a bit skeptical about the practice of re-exporting other libraries. At the very least, I'll request that we don't mutate those libraries' APIs before re-exporting because it seems a bit subversive to me.

I also am curious about the testing situation. It looks like we now point our tests at the lib directory, which requires the build to be run. This definitely works, but a lot of test tools offer a hook for running the test (and source) code through babel in-memory when running the suite which seems a bit nicer.

dan-f · 2016-11-02T14:25:56Z

    "lib",
-    "dist"
+    "dist",
+    "src"


This means that src will be distributed as part of the node package. I don't see any reason for this. People use npm to install JS which they can bundle into their apps, or run on Node, not to browse and edit source code.

If people want to see the source for a given release of solid-client they can browse on github for the tag of that release.

No prob with removing it. I saw that we included the src dir in other libs, but maybe the thing to do is to not include it anywhere.

That's my opinion, at least. It's not a big deal, but it avoids confusion, because sometimes the library consumer want's do do something like require('solid-client/lib/path/to/module'), and if there's a lib and a src, the choice could be ambiguous to that library consumer.

Yeah, agreed. Will remove src/ from the npm files.

dan-f · 2016-11-02T14:27:31Z

+    "src"
  ],
  "scripts": {
+    "babel": "babel src -d lib",


Just a nitpick, but I'd prefer if this was called something like build-lib to indicate that it's the build scrip for the lib directory, rather than being named after the tool it uses.

Also, a general question: were we not using es2015 previously?

We'll have to make the webpack builds use babel as well. I can point you to examples if you like.

Sure, I can change it from babel to build-lib.

Re 'not using es2015 previously' - I think previously it was in the dev dependencies, this PR moves it to regular dependencies.

Oooh. It should definitely be in the devDependencies. The build system is only ever run in the dev environment.

dan-f · 2016-11-02T14:29:32Z

    "preversion": "npm test",
-    "postversion": "git push --follow-tags",
-    "prepublish": "npm run build"
+    "postpublish": "git push --follow-tags",


Why did you change this from postversion to postpublish?

It's my opinion that you'd want to push the git tags whenever bumping the package version, whether or not you subsequently publish the package to npm.

My logic here was -- the tags only make sense in the context of publishing.
If the push is done post-publish, it means that if publish fails, a developer can roll back the versioning slightly easier (locally, without having to change history on the remote repo).

Good point, it's definitely important to make sure that a release works before pushing tags. But I'm pretty sure that's why we have that preversion hook to run the tests.

I don't feel strongly about this one, so I'm fine with this.

On second pass, I actually think it's very important that we push tags whenever versioning, especially for a library that multiple folks work on. If we only ever push tags when publishing it's possible that two different developers bump the version at the same time independently and create a weird state of affairs in our git tree.

That's fine. I'll change the script back to postversion

dan-f · 2016-11-02T15:07:10Z

+const ns = require('solid-namespace')(rdf)
+const acl = require('solid-permissions')
+Object.assign(acl, acl.Authorization.acl)
+acl.ALL_MODES = acl.Authorization.ALL_MODES


I'm of the opinion that it's not really solid-client's responsibility to re-export the libraries it uses, and especially that it shouldn't mutate the APIs of the libraries which it uses.

So in this example, if solid-permissions should export an ALL_MODES constant at the top level, then it's the responsibility of solid-permissions to do so. Though I think that namespacing within acl.Authorization makes sense.

No prob. I can undo this part, and export the constants in solid-permissions.

dan-f · 2016-11-02T15:07:58Z

@@ -1,31 +0,0 @@
-module.exports = `@prefix acl: <http://www.w3.org/ns/auth/acl#>.


What happened here?

This acl-container-ttl.js test resource was left over from when we extracted the ACL code (and its tests + test resources) to its own solid-permission. Just forgot to delete this one.

dan-f · 2016-11-02T15:09:53Z

    loaders: [
-      {
-        test: /\.js$/,
-        // exclude: /(node_modules)/,


Oh, I made a comment above about how we should be using babel in the webpack build, not realizing that we already did. We definitely should not be deleting this, otherwise solid-client won't work in browsers.

I removed it from the webpack config because it's made redundant -- the webpack entry point now points to lib/, which is already pre-transpiled.

I see. I'd still recommend keeping this, and pointing webpack at the src directory because

It removes a dependency on the state of the dev environment

Everyone does it this way (I know, I know...)

dan-f · 2016-12-19T16:38:45Z

-    "postversion": "git push --follow-tags",
-    "prepublish": "npm run build"
+    "postpublish": "git push --follow-tags",
+    "prepublish": "npm run build && npm run test"


As a nitpick I'd rather see tests fail before attempting to run the build.

No prob. I switched the order.

dmitrizagidulin added 4 commits November 1, 2016 14:28

Export solid-permission constants

c3fd59a

Move lib/ to src/

8df47f9

Add babel-cli, update configs to use new lib/ dir

73b37f8

Remove unused acl test resource

6929684

dmitrizagidulin added the ready for review label Nov 1, 2016

dan-f suggested changes Nov 2, 2016

View reviewed changes

dmitrizagidulin mentioned this pull request Nov 22, 2016

addPermission() is undefined on the response from createContainer() #133

Open

dmitrizagidulin added 3 commits November 22, 2016 16:25

Refactor package, webpack config to reflect PR feedback

9b867f8

Bump solid-permissions lib dep

35f2c5f

Push tags on postversion

eccbcaa

dan-f approved these changes Dec 19, 2016

View reviewed changes

Run npm test before building

5f6027b

dmitrizagidulin force-pushed the dz_fix_acl_export branch from 91529f9 to 5f6027b Compare December 19, 2016 17:03

dmitrizagidulin merged commit 96f214d into master Dec 19, 2016

dmitrizagidulin deleted the dz_fix_acl_export branch December 19, 2016 17:07

		@@ -1,31 +0,0 @@
		module.exports = `@prefix acl: <http://www.w3.org/ns/auth/acl#>.

Conversation

dmitrizagidulin commented Nov 1, 2016

Uh oh!

dmitrizagidulin commented Nov 1, 2016

Uh oh!

dan-f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-f Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dan-f Nov 2, 2016 •

edited

Loading