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

Adds section for commonjs support #1989

Merged
merged 3 commits into from
Jun 7, 2017

Conversation

vernak2539
Copy link
Contributor

Provides documentation for using library in commonjs build tools

{% highlight js %}
npm install --save-dev imports-loader
// or
yarn add --dev imports-loader
Copy link
Contributor

Choose a reason for hiding this comment

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

remove yarn reference. npm 5 essentially is yarn. we do not reference yarn elsewhere nor plan to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree to disagree, but done

@swilliamset
Copy link
Contributor

@RodEsp and anyone following #1985 please feel free to provide feedback on these instructions

Copy link
Contributor

@swilliamset swilliamset left a comment

Choose a reason for hiding this comment

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


{% highlight js %}
// webpack 2 config
// add as a rule to module.rule
Copy link

Choose a reason for hiding this comment

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

Should this be added to module.rule or module.loaders?

Copy link

@RodEsp RodEsp Jun 6, 2017

Choose a reason for hiding this comment

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

Oh... my bad.. webpack 2. Sorry.
Do we want to include webpack 1 instructions which would use module.loader instead though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should push for progression. If people need to integrate with webpack 1, they can look it up?

Copy link

Choose a reason for hiding this comment

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

Yeah, sounds good to me.
We need to get devtest updated to webpack 2...

Copy link

@RodEsp RodEsp left a comment

Choose a reason for hiding this comment

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

This looks good to me.
I don't understand how these new code review thingies work lol.

@swilliamset
Copy link
Contributor

pulling it down locally and reading it over again and then i'll merge

Copy link
Contributor

@swilliamset swilliamset left a comment

Choose a reason for hiding this comment

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

In an effort to keep our documentation consistent please you move the comments out to number steps like the UMD section.

image

{% endhighlight %}

{% highlight js %}
// webpack 2 config
Copy link
Contributor

Choose a reason for hiding this comment

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

remove reference to webpack 2

Copy link

Choose a reason for hiding this comment

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

Wait, why did we remove this?
I think it makes perfect sense to document where to place the import loader (whether it's config.rules for webpack 2 or config.loaders for webpack 1). Not everyone is a webpack expert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @RodEsp, we deleted this for the reason of being progressive. The way that the docs state it now is, "add a rule," which references webpack 2. If someone is using webpack 1, they can easily search for a backport.

Copy link

Choose a reason for hiding this comment

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

Got ya. Thanks @vernak2539.
I guess I like to be more explicit since I probably wouldn't have realized webpack 2 was being implied just from that "add rule" before this PR. But you're right, it's not too hard to look up.

<h4>Working with webpack</h4>

{% highlight js %}
npm install --save-dev imports-loader
Copy link
Contributor

Choose a reason for hiding this comment

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

remove --save-dev

new versions of npm save automagically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they "automagically" save to dependencies, not devdeps.

And I don't think we should assume as this (below) makes no illusion to force you to npm v5

  "engines": {
    "node": ">=5.8.x"
  },

@swilliamset swilliamset merged commit ff8e6b5 into ExactTarget:gh-pages Jun 7, 2017
@vernak2539 vernak2539 deleted the gh-pages branch June 7, 2017 15:10
@cmcculloh-kr cmcculloh-kr modified the milestone: 3.16.1 Jun 7, 2017
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