-
Notifications
You must be signed in to change notification settings - Fork 47
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
Get ready to publish Alloy as an NPM repo #637
Conversation
@@ -1,8 +1,13 @@ | |||
{ | |||
"name": "@adobe/alloy", | |||
"version": "2.3.0", | |||
"description": "Client-Side SDK for Unified Data Collection", | |||
"main": "src/core/index.js", | |||
"description": "Adobe Experience Platform Web SDK", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally :)
package.json
Outdated
} | ||
], | ||
"dependencies": { | ||
"css.escape": "^1.5.1", | ||
"typescript": "^4.0.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that pull typescript into the library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I didn't mean to add this as a dependency. Thanks for catching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for moving this forward Jon. I left some comments that we should probably resolve.
README.md
Outdated
window.alloy("sendEvent", { ... }); | ||
``` | ||
|
||
The ES6 exports are exposed via the NPM "module" property, so you may need to adjust your rollup system to look for the module property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"rollup system" should probably be "bundler" or "JavaScript bundler".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions here:
(1) Are you aware of any bundlers that don't support module
by default? I'm aware it isn't standard, but I think it's been supported by default for 2+ years on the major bundlers. If so, this line may not be providing value and just introducing cognitive load, but who knows. If you keep it, it might be better to say something like "are exposed by the NPM package via the module
field of its package.json, so you may...". If I'm a consumer having problems, that might give me more context for googling. 🤷♂️
(2) I'm trying to think through why we're creating main.js
and modules.js
as bundles. Normally what I've seen is that you would just run all the source code files through babel and dump all the resulting files into, say, an es5
directory, then point the main
field in package.json to es5/index.js
. For the es6 variant, you may just be able to point the module
field in package.json to src/index.js
.
One of the benefits of not generating bundles for the npm package consumers is that if the consuming application already has a dependency that matches one of our dependencies, the final application bundle only needs to include the dependency code once. Currently, we only have a couple dependencies, so that chance is somewhat low, but that may grow over time
One effect of taking this approach is that the extension will need to do some bundling before getting packaged and uploaded. But, I believe this would mean that you would be able to move the Launch core module dependencies from peerDependencies
back to dependencies
in this repo. When you do the bundling for the extension, I believe you'd just mark Launch core module dependencies as "externals" so that Launch can provide them instead of bundling them in from node_modules
.
Sorry, I should've thought through this during my previous review as it definitely affects what I was saying in https://github.com/adobe/alloy/pull/632/files#r518529523.
README.md
Outdated
## Installation | ||
|
||
There are three supported ways to use Alloy. | ||
1. Using Adobe Launch, install the Adobe Experience Platform Web SDK Extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically we should change "Adobe Launch" to "Adobe Experience Platform Launch" and remove "the" from before "Adobe Experience Platform Web SDK". We should probably lowercase "Extension" because "Adobe Experience Platform Web SDK Extension" isn't a brand itself and "Extension" alone isn't a pronoun.
README.md
Outdated
2. Use the pre-built, minified version available via a CDN. You could also self-host this version. | ||
3. Use the NPM library which exports an ES6 module. | ||
|
||
### Using the launch extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"launch" here is a secondary reference so it should probably be "Experience Platform Launch" (my understanding is we can can drop the "Adobe" on secondary references).
README.md
Outdated
|
||
### Using the launch extension | ||
|
||
For documentation on the AEP Web SDK Launch Extension see the [launch documentation](https://docs.adobe.com/content/help/en/launch/using/extensions-ref/adobe-extension/aep-extension/overview.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lowercase "Extension" and change "launch" to "Experience Platform Launch".
README.md
Outdated
|
||
For documentation on the AEP Web SDK Launch Extension see the [launch documentation](https://docs.adobe.com/content/help/en/launch/using/extensions-ref/adobe-extension/aep-extension/overview.html) | ||
|
||
### Using the stand alone version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"standalone" is one word.
README.md
Outdated
|
||
The ES6 exports are exposed via the NPM "module" property, so you may need to adjust your rollup system to look for the module property. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think all this documentation should be consolidated into the user docs like I mentioned in #632 (comment). It sounds like you might disagree, but you did mention you were considering at least trimming down this readme to the es6 installation instructions.
7e32b97
to
ca0892a
Compare
@Aaronius can you take another look at this PR? |
@Aaronius can you take another look at this PR? I implemented the createInstance method like you suggested in the docs PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like.
src/index.js
Outdated
export default instanceName => { | ||
const { executeCommand: instance, logger } = createExecuteCommand( | ||
instanceName | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would make more sense to create the "log controller" here and pass it into createExecuteCommand
rather than creating the log controller inside createExecuteCommand
and exporting logger
?
@@ -10,7 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag | |||
governing permissions and limitations under the License. | |||
*/ | |||
|
|||
import createInstance from "../../../../src/core/createInstance"; | |||
import createInstance from "../../../../src/core/createInstanceFunction"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to rename createInstance
to createInstanceFunction
here.
package.json
Outdated
@@ -68,6 +74,8 @@ | |||
"uuid": "^3.3.2" | |||
}, | |||
"devDependencies": { | |||
"@adobe/alloy": "file:./", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now this just references the current directory, but when we publish the NPM library I will change this to reference the actual version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @jonsnyder !!
I recommend testing the built library manually in the sandbox since there's so many changes :)
Description
This PR gets the repo ready to be published as an NPM library. Other PRs will automate the build process. The NPM library publishes an ES6 module which exposes the baseCode, core initialization, and the createEventMergeId functions. The built baseCode and standalone javascript files are not published in the NPM library. If you want to use the built versions you should use the version on the CDN.
See #632
There are three entry points now:
I was also able to remove the REACTOR pre-processor statements, as the launch extension can use the ES6 module and the createEventMergeId directly.
Related Issue
https://jira.corp.adobe.com/browse/CORE-52500
Motivation and Context
Screenshots (if appropriate):
Types of changes
Checklist: