Skip to content
This repository has been archived by the owner on May 9, 2022. It is now read-only.

Fix npm/browserify usage #2

Merged
merged 1 commit into from
Jul 3, 2014
Merged

Conversation

TooTallNate
Copy link
Contributor

Other plugins are doing define('scribe-common/element'), so for this to work in npm/browserify then we need the element.js file to be located at the root of the repository.

Also set the package.json's "main" to element.js, since there is no scribe-common.js file, and element.js appears to be the only JS file in the repo at the moment.

TooTallNate added a commit to Automattic/scribe-plugin-intelligent-unlink-command that referenced this pull request May 25, 2014
@TooTallNate
Copy link
Contributor Author

@OliverJAsh ping

TooTallNate added a commit to Automattic/scribe-plugin-smart-lists that referenced this pull request May 28, 2014
@OliverJAsh
Copy link

Hmm, I’m not sure whether we should do this, or update the RequireJS path to scribe-common in the other plugins: https://github.com/guardian/scribe-plugin-smart-lists/blob/master/Plumbing.js#L16

@TooTallNate
Copy link
Contributor Author

@OliverJAsh Ya I was contemplating that same dilemma. I decided to just fix it here since it would be just 1 pull request instead of n (there's at least 2 plugins using this modules that I can think of off the top of my head).

If you'd rather fix at the plugin level then I'm fine with that as well, and we can send PRs to the individual plugins. Just let me know how you would like to proceed.

@TooTallNate
Copy link
Contributor Author

Ping. With guardian/scribe#195 in the pipeline, I'm gonna need this working.

@OliverJAsh
Copy link

Now that we have two files, it doesn't make sense to change to the main to element.js.

What’s the alternative solution?

@TooTallNate
Copy link
Contributor Author

The alternative is to just remove "main" completely. In all usage I've seen of this module so far, a sub-file is required directly instead of the main package ("scribe-common/node"), so we can safely remove it.

@OliverJAsh
Copy link

I’m happy to do that if you are?

@TooTallNate
Copy link
Contributor Author

@OliverJAsh Sure thing. PR updated.

@TooTallNate
Copy link
Contributor Author

Bump.

@lrowe
Copy link

lrowe commented Jun 30, 2014

I'd like to promote #4 as a better alternative to this as it makes scribe-common/element and scribe-common/node commonjs modules. (See my post to the google group.)

@OliverJAsh
Copy link

The src folder is useful to have. I don't want to remove that.

The issue is that, with the current setup, the browserify require points to node_modules/scribe-common (npm module) whereas the RequireJS require points to bower_components/scribe-common/src. Thus, a require for scribe-common/element will point to:

  • node_modules/scribe-common/element.js in browserify
  • bower_components/scribe-common/src/element.js in RequireJS

I imagine there would be similar issues when scribe-plugin-sanitizer tries to load html-janitor: https://github.com/guardian/scribe-plugin-sanitizer/blob/master/src/scribe-plugin-sanitizer.js#L2

I propose that we:

  1. Remove the main from package.json, as you have done here, so that a require for scribe-common in browserify will try to load scribe-common/index.js (non-existent)
  2. Update the RequireJS path for scribe-common from bower_components/scribe-common/src to bower_components/scribe-common so that requiring in both browserify and RequireJS will consistently try to load the same file
  3. Update all scribe-common require calls to lookup files in the src folder, e.g. define(['scribe-common/element'], …) => define(['scribe-common/src/element'], …)

Whatever solution we go with, we should use it consistently across the Scribe ecosystem.

All of this sucks 😒

@TooTallNate
Copy link
Contributor Author

@OliverJAsh Albeit taking the most work, that does indeed sound like the best solution to me.

@samvasko
Copy link

samvasko commented Jul 1, 2014

Oliver, but some modules are using element that is directly is scribe repository. What's up with that?

@OliverJAsh
Copy link

That's a different element module, so that's fine.

The file `src/scribe-common.js` doesn't exist.
@TooTallNate
Copy link
Contributor Author

@OliverJAsh Ok, so this PR has been reduced to just removing the main field from the package.json. All other "fixes" need to be done in the other repos.

OliverJAsh pushed a commit that referenced this pull request Jul 3, 2014
Fix npm/browserify usage
@OliverJAsh OliverJAsh merged commit 03b75b8 into guardian:master Jul 3, 2014
@OliverJAsh
Copy link

v0.0.5 is out!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants